Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix menu items display with 6 columns + fix disappear after exiting #999

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

uazo
Copy link
Collaborator

@uazo uazo commented Mar 10, 2021

fix #963 + see comment #570 (comment)

@uazo
Copy link
Collaborator Author

uazo commented Mar 11, 2021

Assuming that what I wrote is correct, I would like to make you a proposal on how to write this patch: I think we can try a next step, componentization.

The goal would be to create no more a .patch file, but, using DEPS, hooks and .gns, separate this code from the base code, in some way by exploiting the logic that chromium makes available to us + those that seem tested and used in brave.

It's a bit a revolution but the benefits could be:

  1. improve reviews (no longer based on patches, but directly on the code, which becomes easily comparable)
  2. potentially facilitate the transition between one version and another of chromium
  3. in all probability simplify the development of tests

these are the steps I imagine:

  1. setup of a folder /src with a structure to be agreed, I think that one as similar as possible to the one in use in chromium is an advantage
  2. definition of a DEPS for gclient that downloads our folder in third_party during the sync phase
  3. definition of a hook that executes a git-am with a patch to be built that allow us to fit our code in the base one, where it is not foreseen.
    for example I am thinking of the simple modification of functions as virtual, or the introduction of the use of dependency injection constructs or the factory pattern where not foreseen, or simply by calling our functions instead of those present in the code, the best way will only be seen by trying
  4. use of all the mechanisms already present in the code for embedders (I think I understand that they are called that)

the c++ part seems to me the most developed in this sense, I have seen this logic many times.
the java part, on the other hand, is a bit tougher, but lately I seem to have seen that they are also moving in this direction.

what do you say, shall we try?

@csagan5
Copy link
Contributor

csagan5 commented Mar 12, 2021

The goal would be to create no more a .patch file, but, using DEPS, hooks and .gns, separate this code from the base code, in some way by exploiting the logic that chromium makes available to us + those that seem tested and used in brave.

I am skeptical: what will be the outcome? Will there be no changes needed to Chromium source or less changes?

  1. improve reviews (no longer based on patches, but directly on the code, which becomes easily comparable)

I do not think all patches can become components.

3. definition of a hook that executes a git-am with a patch to be built that allow us to fit our code in the base one

I think this is a bad idea. Patches are code changes, hooks are programs.

what do you say, shall we try?

You can try for one of these and then I will evaluate; but no guarantees that it will be merged.

@uazo
Copy link
Collaborator Author

uazo commented Mar 12, 2021

I am skeptical: what will be the outcome? Will there be no changes needed to Chromium source or less changes?

this obviously I do not know a priori, however, the goal is not less changes, but easier maintenance

I do not think all patches can become components.

you have to try, obviously not the ungoogled patches, but those seem more easy to handle to me. In this regard, if you have time to check the ones I produced in my tests, it's seem more complete to me, therefore no need to apply the Remove-binary-blob-integrations and can be verified one by one (although the hypothesis of aggregating them is the best for me).

hooks are programs.

hooks can also be bash scripts (https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/gclient.py#236)

#     "action"   A list describing a command to run along with its arguments, if
#                any.  An action command will run at most one time per gclient
#                invocation, regardless of how many files matched the pattern.
#                The action is executed in the same directory as the .gclient
#                file.  If the first item in the list is the string "python",
#                the current Python interpreter (sys.executable) will be used
#                to run the command. If the list contains string
#                "$matching_files" it will be removed from the list and the list
#                will be extended by the list of matching files.

You can try for one of these and then I will evaluate; but no guarantees that it will be merged.

no problem, just take it into consideration

@csagan5 csagan5 merged commit 167f960 into bromite:master Mar 12, 2021
@uazo uazo deleted the fix-963 branch July 18, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu items are not properly displayed with tab overflow menu regroup flag
2 participants