Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552663002:
NavigationController: merge Reload*() to one Reload() with ReloadType (Closed)
|
Created:
4 years ago by Takashi Toyoshima Modified:
4 years ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNavigationController: merge Reload*() to one Reload() with ReloadType
Now, NavigationController has multiple Reload*() methods to trigger
various kinds of reload operations. But this is confusing because it
takes one boolean argument, and in some contexts, one boolean argument
for reload means a flag to bypass caches.
Also, using one method and calling it with ReloadType flag is more
consistent with other navigation related implementation in Blink.
This is the first patch to introduce new Reload method with ReloadType
argument. Once this patch is submitted, other changes will follow to
call the new Reload from several places, and remove Reload* methods.
BUG=670232
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=jam@chromium.org
Committed: https://crrev.com/c3d6560ac8ac49a766af35240919ab90eb808d1c
Cr-Commit-Position: refs/heads/master@{#437517}
Patch Set 1 #
Total comments: 5
Patch Set 2 : review #5 #
Total comments: 1
Patch Set 3 : [rebase] #
Dependent Patchsets: Messages
Total messages: 23 (13 generated)
Description was changed from ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 ========== to ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
toyoshim@chromium.org changed reviewers: + creis@chromium.org
creis@, what do you think about this? I prefer to merge all Reload* methods to one Reload method with ReloadType argument though it affects many caller files.
I'm ok with this-- I think the current approach made more sense when there were fewer variants. Thanks for the cleanup! That said, let's include at least one call-site conversion outside content/ in this CL, to justify the content/public change. We have a rule not to add public methods until there's at least one caller of them. https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... File content/public/browser/navigation_controller.h (right): https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... content/public/browser/navigation_controller.h:353: // Reloads the current entry under the speicified ReloadType. This will nit: specified nit: We don't need the second sentence, since the TODO covers that. https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... content/public/browser/navigation_controller.h:359: virtual void Reload(bool check_for_repost, ReloadType reload_type) = 0; I'm wondering if it would be worth reversing the order of the parameters, since |reload_type| seems like the more fundamental one for affecting behavior. (On the flip side, I see that ReloadInternal is already set up the other way, so this is a smaller delta.) I'll leave it up to you, for which one you think is clearer to callers.
https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... File content/public/browser/navigation_controller.h (right): https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... content/public/browser/navigation_controller.h:353: // Reloads the current entry under the speicified ReloadType. This will On 2016/12/05 19:34:00, Charlie Reis wrote: > nit: specified > nit: We don't need the second sentence, since the TODO covers that. Done. https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... content/public/browser/navigation_controller.h:359: virtual void Reload(bool check_for_repost, ReloadType reload_type) = 0; One pros of current order is that we can set a default type on finalizing this cleanup. It would be virtual void Reload(bool check_for_repost, ReloadType reload_type = ReloadType::NORMAL); I assume NORMAL in the final stage mean MAIN_RESOURCE of today. If adding a default type isn't good idea here, I completely agreed that reversed order is better. https://codereview.chromium.org/2552663002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (left): https://codereview.chromium.org/2552663002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:267: new_tab->GetController().ReloadBypassingCache(true); randomly picked up one caller in chrome/
Description was changed from ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=msw@chromium.org ==========
creis@chromium.org changed reviewers: + jam@chromium.org
Thanks! LGTM. jam@, can you give an owners stamp for the chrome/ change? https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... File content/public/browser/navigation_controller.h (right): https://codereview.chromium.org/2552663002/diff/1/content/public/browser/navi... content/public/browser/navigation_controller.h:359: virtual void Reload(bool check_for_repost, ReloadType reload_type) = 0; On 2016/12/06 08:10:42, toyoshim wrote: > One pros of current order is that we can set a default type on finalizing this > cleanup. > > It would be > > virtual void Reload(bool check_for_repost, ReloadType reload_type = > ReloadType::NORMAL); > > I assume NORMAL in the final stage mean MAIN_RESOURCE of today. > > If adding a default type isn't good idea here, I completely agreed that reversed > order is better. I'm generally not a fan of default arguments, but it could be useful in a case like this if the vast majority of cases use the default. I'm ok with leaving the order as is if we're considering that.
Description was changed from ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=msw@chromium.org ========== to ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=jam@chromium.org ==========
Since the chrome side change is caller side mechanical one, I'd use TBR here.
The CQ bit was checked by toyoshim@chromium.org
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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 toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2552663002/#ps40001 (title: "[rebase]")
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": 1481273527651310,
"parent_rev": "9316bbaa06a8353a00a64fa3959adb74de3bc94a", "commit_rev":
"6c6e5c2553e7892a718c016eeb1863e9da091846"}
Message was sent while issue was closed.
Description was changed from ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=jam@chromium.org ========== to ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=jam@chromium.org Review-Url: https://codereview.chromium.org/2552663002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=jam@chromium.org Review-Url: https://codereview.chromium.org/2552663002 ========== to ========== NavigationController: merge Reload*() to one Reload() with ReloadType Now, NavigationController has multiple Reload*() methods to trigger various kinds of reload operations. But this is confusing because it takes one boolean argument, and in some contexts, one boolean argument for reload means a flag to bypass caches. Also, using one method and calling it with ReloadType flag is more consistent with other navigation related implementation in Blink. This is the first patch to introduce new Reload method with ReloadType argument. Once this patch is submitted, other changes will follow to call the new Reload from several places, and remove Reload* methods. BUG=670232 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=jam@chromium.org Committed: https://crrev.com/c3d6560ac8ac49a766af35240919ab90eb808d1c Cr-Commit-Position: refs/heads/master@{#437517} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c3d6560ac8ac49a766af35240919ab90eb808d1c Cr-Commit-Position: refs/heads/master@{#437517} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
