Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2940743002:
Reorder the methods in JumpList class according to the update workflow (Closed)
|
Created:
3 years, 6 months ago by chengx Modified:
3 years, 6 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReorder the methods in JumpList class according to the update workflow
This CL reorders the methods in JumpList class so that they are laid
out approximately in the order they're invoked. The static methods are
moved below all other methods since they potentially live somewhere
else, e.g., in an unnamed namespace or in a separate class that could
be unit-tested independently. After this reordering, the JumpList code
should be easier to follow and the JumpList update process should be
easier to understand.
BUG=732980
Review-Url: https://codereview.chromium.org/2940743002
Cr-Commit-Position: refs/heads/master@{#480101}
Committed: https://chromium.googlesource.com/chromium/src/+/78b03b6957876df0bb887486a085cc77c9b52898
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix the order of two methods #
Total comments: 6
Patch Set 3 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into reorderjumplistapis #Patch Set 4 : More reordering #Messages
Total messages: 33 (26 generated)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reorder apis according to the jumplist upate process BUG= ========== to ========== Reorder methods according to the jumplist update workflow BUG= ==========
Description was changed from ========== Reorder methods according to the jumplist update workflow BUG= ========== to ========== Reorder methods according to the jumplist update workflow BUG=732980 ==========
Description was changed from ========== Reorder methods according to the jumplist update workflow BUG=732980 ========== to ========== Reorder the methods in JumpList class according to the update workflow This CL reorders the methods in JumpList class so that they are laid out approximately in the order they're invoked. The static methods are moved below all other methods since they potentially live somewhere else, e.g., in an unnamed namespace or in a separate class that could be unit-tested independently. After this reordering, the code should be easier to follow and the JumpList update process should be easier to understand. BUG=732980 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Reorder the methods in JumpList class according to the update workflow This CL reorders the methods in JumpList class so that they are laid out approximately in the order they're invoked. The static methods are moved below all other methods since they potentially live somewhere else, e.g., in an unnamed namespace or in a separate class that could be unit-tested independently. After this reordering, the code should be easier to follow and the JumpList update process should be easier to understand. BUG=732980 ========== to ========== Reorder the methods in JumpList class according to the update workflow This CL reorders the methods in JumpList class so that they are laid out approximately in the order they're invoked. The static methods are moved below all other methods since they potentially live somewhere else, e.g., in an unnamed namespace or in a separate class that could be unit-tested independently. After this reordering, the JumpList code should be easier to follow and the JumpList update process should be easier to understand. BUG=732980 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
This is the CL for reordering the methods. PTAL, thanks!
https://codereview.chromium.org/2940743002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2940743002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:641: void JumpList::Shutdown() { please put all methods here in the same order as those in the .h
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've addressed the comment in the new patch set. PTAL, thanks! https://codereview.chromium.org/2940743002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2940743002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:641: void JumpList::Shutdown() { On 2017/06/14 10:54:12, grt (UTC plus 2) wrote: > please put all methods here in the same order as those in the .h Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:253: void JumpList::Shutdown() { please move up to line 201 https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:65: void Shutdown() override; nit: this method is less interesting to the reader than Enabled(), so it seems logical that it would come later in the class definition. https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:158: void OnIncognitoAvailabilityChanged(); nit: this is more similar to the overrides above, in that it handles notifications coming in from the rest of Chrome. i think it makes sense for it to be up after the TabRestoreServiceObserver overrides.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've addressed all the comments in the new patch set. Thanks! https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:253: void JumpList::Shutdown() { On 2017/06/14 20:50:38, grt (UTC plus 2) wrote: > please move up to line 201 Done. https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:65: void Shutdown() override; On 2017/06/14 20:50:38, grt (UTC plus 2) wrote: > nit: this method is less interesting to the reader than Enabled(), so it seems > logical that it would come later in the class definition. Done. https://codereview.chromium.org/2940743002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:158: void OnIncognitoAvailabilityChanged(); On 2017/06/14 20:50:38, grt (UTC plus 2) wrote: > nit: this is more similar to the overrides above, in that it handles > notifications coming in from the rest of Chrome. i think it makes sense for it > to be up after the TabRestoreServiceObserver overrides. Done.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2940743002/#ps80001 (title: "More reordering")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497636648708060,
"parent_rev": "52dd5c4fd4a98a209ed3bfb8eadf4caf9e40fed6", "commit_rev":
"78b03b6957876df0bb887486a085cc77c9b52898"}
Message was sent while issue was closed.
Description was changed from ========== Reorder the methods in JumpList class according to the update workflow This CL reorders the methods in JumpList class so that they are laid out approximately in the order they're invoked. The static methods are moved below all other methods since they potentially live somewhere else, e.g., in an unnamed namespace or in a separate class that could be unit-tested independently. After this reordering, the JumpList code should be easier to follow and the JumpList update process should be easier to understand. BUG=732980 ========== to ========== Reorder the methods in JumpList class according to the update workflow This CL reorders the methods in JumpList class so that they are laid out approximately in the order they're invoked. The static methods are moved below all other methods since they potentially live somewhere else, e.g., in an unnamed namespace or in a separate class that could be unit-tested independently. After this reordering, the JumpList code should be easier to follow and the JumpList update process should be easier to understand. BUG=732980 Review-Url: https://codereview.chromium.org/2940743002 Cr-Commit-Position: refs/heads/master@{#480101} Committed: https://chromium.googlesource.com/chromium/src/+/78b03b6957876df0bb887486a085... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/78b03b6957876df0bb887486a085... |
