Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954173002:
Remove redundant JumpListUpdater::IsEnabled calls (Closed)
|
Created:
3 years, 6 months ago by chengx Modified:
3 years, 5 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove redundant JumpListUpdater::IsEnabled calls
JumpListUpdater::IsEnabled is checked in JumpList constructor. There
is no need to check it again in JumpList member functions as its state
doesn't change after the JumpList object is constructed.
BUG=738126
Review-Url: https://codereview.chromium.org/2954173002
Cr-Commit-Position: refs/heads/master@{#483719}
Committed: https://chromium.googlesource.com/chromium/src/+/214f4ffc34417cfd3940cab8a73a8faf4a81cebb
Patch Set 1 #
Total comments: 2
Patch Set 2 : Revert some changes #Patch Set 3 : Git pull #Messages
Total messages: 29 (23 generated)
Description was changed from ========== Move JumpListUpdater::IsEnabled towards the very beginning of jumplist update workflow BUG=717236 ========== to ========== Move JumpListUpdater::IsEnabled to the beginning of jumplist update This CL moves the check of JumpListUpdater::IsEnabled from down in the middle to the very beginning of the jumplist update. This saves a lot of unnecessary favicon loading when jumplist is disabled. BUG=717236 ==========
Description was changed from ========== Move JumpListUpdater::IsEnabled to the beginning of jumplist update This CL moves the check of JumpListUpdater::IsEnabled from down in the middle to the very beginning of the jumplist update. This saves a lot of unnecessary favicon loading when jumplist is disabled. BUG=717236 ========== to ========== Move JumpListUpdater::IsEnabled to the beginning of jumplist update This CL moves the check of JumpListUpdater::IsEnabled from down in the middle to the very beginning of the jumplist update. This saves a lot of unnecessary favicon loading and related when jumplist is disabled. BUG=717236 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
PTAL, thanks! This was mentioned in crrev/2941323002.
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.
https://codereview.chromium.org/2954173002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2954173002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:269: if (!JumpListUpdater::IsEnabled()) BrowserView::InitViews only constructs a JumpList if IsEnabled() returns true. is it enough to put a DCHECK(IsEnabled()) in the ctor?
Description was changed from ========== Move JumpListUpdater::IsEnabled to the beginning of jumplist update This CL moves the check of JumpListUpdater::IsEnabled from down in the middle to the very beginning of the jumplist update. This saves a lot of unnecessary favicon loading and related when jumplist is disabled. BUG=717236 ========== to ========== Remove redundant call of JumpListUpdater::IsEnabled JumpListUpdater::IsEnabled is verified via DCHECK() in JumpList constructor. There is no need to check it again in JumpList member functions as the enable state won't change in between. BUG=717236 ==========
Description was changed from ========== Remove redundant call of JumpListUpdater::IsEnabled JumpListUpdater::IsEnabled is verified via DCHECK() in JumpList constructor. There is no need to check it again in JumpList member functions as the enable state won't change in between. BUG=717236 ========== to ========== Remove redundant call of JumpListUpdater::IsEnabled JumpListUpdater::IsEnabled is verified via DCHECK() in JumpList constructor. There is no need to check it again in JumpList member functions as the enable state won't change in between. BUG=738126 ==========
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...
PTAL the new patch set. Thanks! https://codereview.chromium.org/2954173002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2954173002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:269: if (!JumpListUpdater::IsEnabled()) On 2017/06/24 20:42:44, grt (UTC plus 2) wrote: > BrowserView::InitViews only constructs a JumpList if IsEnabled() returns true. > is it enough to put a DCHECK(IsEnabled()) in the ctor? That should be enough.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 ========== Remove redundant call of JumpListUpdater::IsEnabled JumpListUpdater::IsEnabled is verified via DCHECK() in JumpList constructor. There is no need to check it again in JumpList member functions as the enable state won't change in between. BUG=738126 ========== to ========== Remove redundant JumpListUpdater::IsEnabled calls JumpListUpdater::IsEnabled is checked in JumpList constructor. There is no need to check it again in JumpList member functions as the its state doesn't change after the JumpList object is constructed. BUG=738126 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by chengx@chromium.org
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": 40001, "attempt_start_ts": 1498838503758550,
"parent_rev": "ce311c79cd3f0ea928d19abb9001137d113e6705", "commit_rev":
"f4b6d55e6faa3467de9d187d003d57db2c80e78d"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498838503758550,
"parent_rev": "4782af57f14cd7acf259b381d4b47f1d6974d56c", "commit_rev":
"214f4ffc34417cfd3940cab8a73a8faf4a81cebb"}
Message was sent while issue was closed.
Description was changed from ========== Remove redundant JumpListUpdater::IsEnabled calls JumpListUpdater::IsEnabled is checked in JumpList constructor. There is no need to check it again in JumpList member functions as the its state doesn't change after the JumpList object is constructed. BUG=738126 ========== to ========== Remove redundant JumpListUpdater::IsEnabled calls JumpListUpdater::IsEnabled is checked in JumpList constructor. There is no need to check it again in JumpList member functions as the its state doesn't change after the JumpList object is constructed. BUG=738126 Review-Url: https://codereview.chromium.org/2954173002 Cr-Commit-Position: refs/heads/master@{#483719} Committed: https://chromium.googlesource.com/chromium/src/+/214f4ffc34417cfd3940cab8a73a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/214f4ffc34417cfd3940cab8a73a...
Message was sent while issue was closed.
Description was changed from ========== Remove redundant JumpListUpdater::IsEnabled calls JumpListUpdater::IsEnabled is checked in JumpList constructor. There is no need to check it again in JumpList member functions as the its state doesn't change after the JumpList object is constructed. BUG=738126 Review-Url: https://codereview.chromium.org/2954173002 Cr-Commit-Position: refs/heads/master@{#483719} Committed: https://chromium.googlesource.com/chromium/src/+/214f4ffc34417cfd3940cab8a73a... ========== to ========== Remove redundant JumpListUpdater::IsEnabled calls JumpListUpdater::IsEnabled is checked in JumpList constructor. There is no need to check it again in JumpList member functions as its state doesn't change after the JumpList object is constructed. BUG=738126 Review-Url: https://codereview.chromium.org/2954173002 Cr-Commit-Position: refs/heads/master@{#483719} Committed: https://chromium.googlesource.com/chromium/src/+/214f4ffc34417cfd3940cab8a73a... ========== |
