New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 670232 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 670196

Blocking:
issue 612701



Sign in to add a comment

Reload Reloaded: cleanup

Project Member Reported by toyoshim@chromium.org, Dec 1 2016

Issue description

The feature has been enabled for all users from m54, and it reaches to Stable channel.
Let's remove experimental code and do some cleanups.
 
Remaining task list.

1. Remove the feature flag, and Blink runtime flag
2. Add more cache tests into content_browsertests expecially for cases DevTools open
3. Merge Reload variant methods in NavigationController
4. Start next study to replace location.reload()

Blocking: 612701
Blockedon: 670196
Also 670196 could be a sub task of this.
One more related TODO exists in FrameLoader.cpp.
Check if we should restore scroll position for FrameLoadTypeInitialHistoryLoad.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/980f117021a4247387809df1eb1ae27871284a2e

commit 980f117021a4247387809df1eb1ae27871284a2e
Author: toyoshim <toyoshim@chromium.org>
Date: Fri Dec 02 08:15:53 2016

Reload Reloaded: remove experimental flag that has been enabled by default

feastures::kNonValidatingReloadOnNormalReload has been enabled for all
users from m54, and now it reached to Stable channel.
Let's remove experimental flag so to make some cleanups.

BUG= 670232 , 591245
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2541363002
Cr-Commit-Position: refs/heads/master@{#435881}

[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/chrome/app/generated_resources.grd
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/chrome/browser/about_flags.cc
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/content/browser/loader/reload_cache_control_browsertest.cc
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/content/child/runtime_features.cc
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/content/public/common/content_features.cc
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/content/public/common/content_features.h
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/testing/variations/fieldtrial_testing_config.json
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
[modify] https://crrev.com/980f117021a4247387809df1eb1ae27871284a2e/third_party/WebKit/public/web/WebRuntimeFeatures.h

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b887e7c4d46340e170ab47c12b3a0d75433250af

commit b887e7c4d46340e170ab47c12b3a0d75433250af
Author: toyoshim <toyoshim@chromium.org>
Date: Mon Dec 05 07:53:26 2016

NavigationController: remove ReloadToRefreshContent

Now that Reload is equivalent to ReloadToRefreshContent, let's
deprecated ReloadToRefreshContent.

In this change, I do not remove actual caller code that existed
in Android port, because it was already removed accidentaly
in the following refactoring change.
https://codereview.chromium.org/2416723002

Because we already migrated to using ReloadType::MAIN_RESOURCE
for both reloads fortunately, it didn't cause any regression,
and we can safely remove this interface now.

BUG= 612080 ,  670232 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=miguelg@chromium.org

Review-Url: https://codereview.chromium.org/2544283002
Cr-Commit-Position: refs/heads/master@{#436242}

[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
[modify] https://crrev.com/b887e7c4d46340e170ab47c12b3a0d75433250af/content/public/browser/navigation_controller.h

1. Remove the feature flag, and Blink runtime flag => Done
2. Add more cache tests into content_browsertests expecially for cases DevTools open => On review
3. Merge Reload variant methods in NavigationController => Partially done
4. Start next study to replace location.reload() => Not yet
5. TODO exists in FrameLoader.cpp. => Not yet
1 and 2 => Done

3 => split into multiple changes
 3.1 Remove ReloadToRefreshContent => Done
 3.2 Remove ReloadType::MAIN_RESOURCE => On review
 3.3 Remove FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE => Not yet
 3.4 Merge NavigationController::Reload* into ::Reload with ReloadType => On review

4. Start next study to replace location.reload() => Not yet
5. TODO exists in FrameLoader.cpp. => Not yet
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a27638a0c9a2677e92d024a733b6c4884d48f0e3

commit a27638a0c9a2677e92d024a733b6c4884d48f0e3
Author: toyoshim <toyoshim@chromium.org>
Date: Tue Dec 06 10:20:47 2016

Reload cache control test: add more tests

Add new test case for the same page navigation.
Also, add additional checks with DevTool shown/closed
for each test case.

BUG= 670232 ,  671545 
TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*'
TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation

Review-Url: https://codereview.chromium.org/1991153002
Cr-Commit-Position: refs/heads/master@{#436559}

[modify] https://crrev.com/a27638a0c9a2677e92d024a733b6c4884d48f0e3/content/browser/loader/reload_cache_control_browsertest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0d4eed34b7a81eba62fa16f0e7e984cb587ad21

commit b0d4eed34b7a81eba62fa16f0e7e984cb587ad21
Author: toyoshim <toyoshim@chromium.org>
Date: Fri Dec 09 06:03:04 2016

content::ReloadType cleanup: make NORMAL behave as MAIN_RESOURCE

Now that ReloadType::MAIN_RESOURCE is default reload behavior and
we do not need to use ReloadType::NORMAL any more, let's make
ReloadType::NORMAL behavior same with one of ReloadType::MAIN_RESOURCE.

BUG= 670232 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2557633003
Cr-Commit-Position: refs/heads/master@{#437481}

[modify] https://crrev.com/b0d4eed34b7a81eba62fa16f0e7e984cb587ad21/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/b0d4eed34b7a81eba62fa16f0e7e984cb587ad21/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/b0d4eed34b7a81eba62fa16f0e7e984cb587ad21/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/b0d4eed34b7a81eba62fa16f0e7e984cb587ad21/content/common/frame_message_enums.h
[modify] https://crrev.com/b0d4eed34b7a81eba62fa16f0e7e984cb587ad21/content/public/browser/reload_type.h

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3d6560ac8ac49a766af35240919ab90eb808d1c

commit c3d6560ac8ac49a766af35240919ab90eb808d1c
Author: toyoshim <toyoshim@chromium.org>
Date: Fri Dec 09 11:23:17 2016

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
Cr-Commit-Position: refs/heads/master@{#437517}

[modify] https://crrev.com/c3d6560ac8ac49a766af35240919ab90eb808d1c/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/c3d6560ac8ac49a766af35240919ab90eb808d1c/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/c3d6560ac8ac49a766af35240919ab90eb808d1c/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/c3d6560ac8ac49a766af35240919ab90eb808d1c/content/public/browser/navigation_controller.h

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6142d96f1502d098a9945ff69499321ca48e691b

commit 6142d96f1502d098a9945ff69499321ca48e691b
Author: toyoshim <toyoshim@chromium.org>
Date: Mon Dec 19 09:07:25 2016

NavigationController: Reload methods migration

Use the new Reload() method with a ReloadType from everywhere, and
remove old Reload*() methods.

BUG= 670232 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=bajones@chromium.org, jam@chromium.org

Review-Url: https://codereview.chromium.org/2561983002
Cr-Commit-Position: refs/heads/master@{#439439}

[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/blimp/engine/session/tab.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/android/vr_shell/vr_shell.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/autofill/autofill_interactive_uitest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/captive_portal/captive_portal_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/captive_portal/captive_portal_tab_reloader.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/extensions/extension_action_runner.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/extensions/extension_action_runner_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/extensions/navigation_observer.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/gpu/three_d_api_observer.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/history/history_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/interstitials/chrome_controller_client.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/memory/tab_manager_unittest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/plugins/flash_permission_context.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/plugins/plugin_observer.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/previews/previews_infobar_delegate.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/renderer_context_menu/render_view_context_menu.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/repost_form_warning_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/browser_instant_controller.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/cocoa/applescript/tab_applescript.mm
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/collected_cookies_infobar_delegate.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/sad_tab.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/tabs/tab_strip_model.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/chrome/browser/ui/website_settings/website_settings_infobar_delegate.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/child_process_security_policy_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/devtools/protocol/page_handler.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/devtools/render_frame_devtools_agent_host.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/frame_host/navigation_controller_impl_unittest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/loader/resource_dispatcher_host_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/browser/web_contents/web_contents_view_aura_browsertest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/public/browser/navigation_controller.h
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/public/browser/reload_type.h
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/public/test/test_renderer_host.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/shell/browser/shell.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/content/shell/browser/shell_web_contents_view_delegate_mac.mm
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/extensions/browser/guest_view/web_view/web_view_apitest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/services/navigation/public/interfaces/view.mojom
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/services/navigation/view_impl.cc
[modify] https://crrev.com/6142d96f1502d098a9945ff69499321ca48e691b/services/navigation/view_impl.h

3 => split into multiple changes
 3.2 Remove ReloadType::MAIN_RESOURCE => Done
 3.3 Remove FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE => Not yet
 3.4 Merge NavigationController::Reload* into ::Reload with ReloadType => Done

4. Start next study to replace location.reload() => Done in a separate crbug
5. TODO exists in FrameLoader.cpp. => Not yet
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0299ae01c9ecd229d991d899bff694232a0ea579

commit 0299ae01c9ecd229d991d899bff694232a0ea579
Author: toyoshim <toyoshim@chromium.org>
Date: Tue Dec 20 15:22:32 2016

FrameLoader: remove a TODO and add a comment

Remove a TODO that I left before because I confirmed that
current code is correct, and chaging it makes many unit tests
fail.

BUG= 670232 

Review-Url: https://codereview.chromium.org/2587323002
Cr-Commit-Position: refs/heads/master@{#439807}

[modify] https://crrev.com/0299ae01c9ecd229d991d899bff694232a0ea579/third_party/WebKit/Source/core/loader/FrameLoader.cpp

Project Member

Comment 15 by bugdroid1@chromium.org, Jan 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/63de737e764be064c7a3d28a5463f68abcbde975

commit 63de737e764be064c7a3d28a5463f68abcbde975
Author: toyoshim <toyoshim@chromium.org>
Date: Fri Jan 13 08:31:41 2017

Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

This change introduces two functional changes:

 1. RELOAD_ORIGINAL_REQUEST_URL is changed to revalidate
    only main resource instead of revalidating all.
    (in render_frame_impl.cc ReloadFrameLoadTypeFor)

 2. In LoadDataURL(), replace flag is set for usual reload
    operations. This was the original behavior, but unexpectedly
    changed in newly introduced ReloadMainResource cases.

BUG= 670232 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2583123002
Cr-Commit-Position: refs/heads/master@{#443518}

[modify] https://crrev.com/63de737e764be064c7a3d28a5463f68abcbde975/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/63de737e764be064c7a3d28a5463f68abcbde975/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/63de737e764be064c7a3d28a5463f68abcbde975/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/63de737e764be064c7a3d28a5463f68abcbde975/content/common/frame_message_enums.h
[modify] https://crrev.com/63de737e764be064c7a3d28a5463f68abcbde975/content/renderer/render_frame_impl.cc

Status: Fixed (was: Started)
3.3 Remove FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE => Done by #c15
5. TODO exists in FrameLoader.cpp. => Done by #c14

Sign in to add a comment