[Devtools] Fix reload behavior when devtools is open

third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp

Index: third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp
diff --git a/third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp b/third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp
index 9c6aca3117d6cee362021f6b2d0087b8ce3fa668..ed761df49d4f59554a25f68fdb58992f10da436b 100644
--- a/third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp
+++ b/third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp
@@ -416,7 +416,7 @@ void InspectorPageAgent::reload(ErrorString*, const Maybe<bool>& optionalBypassC
     m_pendingScriptToEvaluateOnLoadOnce = optionalScriptToEvaluateOnLoad.fromMaybe("");
     m_v8Session->setSkipAllPauses(true);
     m_reloading = true;
-    m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect);

[Takashi Toyoshima, 2016/08/25 08:00:34]

When is this method called?

Now, FrameLoadTypeReload is a default behavior, but
FrameLoadTypeReloadMainResource is used behind a flag. If this is called when
user push reload button on the DevTools UI, or on similar situation via a remote
control, I think it's better to change this when we enable the flag by default.

If so, can you leave following comment for me?
TODO(crbug.com/612701): Change reload type when new reload implementation is
enabled by default.

If this is triggered even when a user clicks reload button from Chrome UI, this
will fix a known issue that I need to fix for the new reload.

[Takashi Toyoshima, 2016/08/25 10:52:39]

I checked by myself. This will fix the issue that means this method is called
for reload UI.

So the right fix is plumbing the flag from the browser process, and switch
reload mode here, but probably it's enough to leave a TODO and update the mode
when the feature is enabled by default.

[allada, 2016/08/25 17:30:10]

I see that's where I was confused, in my ToT version of chromium it was on by
default. I manually disabled it and it goes back to FrameLoadTypeReload
behavior.

So would you prefer a TODO with your name or would you like me to plum it? I
think it may be worth to write a browser test to ensure devtools does not change
behavior of reload instead of doing it in protocol (since protocol causes the
bug). I have a feeling a little bit of refactoring in the way devtools handles
reload is due at some point though.

[Takashi Toyoshima, 2016/08/26 09:39:55]

Yes, if the setting is default, 50% of ToT/Canary/Dev and 10% of Beta users get
a new behavior.

About plumbing or TODO, I'm fine with both. It's up to you.

For testing, I agreed that we should have tests for reload, and actually I
partially added it recently as a content browser test.
content/browser/loader/reload_cache_control_browsertest.cc is the one. It has a
TODO to have additional tests with DevTools open. I planned to add it when I fix
this issue, but it hasn't happened yet.

[Takashi Toyoshima, 2016/08/29 11:55:36]

FYI, I will plumb the flag for another purpose.
See patch set 5 of https://codereview.chromium.org/2257743002/.

If you need this flag and will submit before my change, please feel free to use
the code in this CL.

[Takashi Toyoshima, 2016/09/07 06:48:20]

Now the flag is available in Blink.
So, this could be 

RuntimeEnabledFeatures::reloadwithoutSubResourceCacheRevalidationEnabled() ?
FrameLoadTypeReloadMainResource : FrameLoadTypeReload

[allada, 2016/09/07 21:15:36]

Done.

+    m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReloadMainResource, ClientRedirectPolicy::NotClientRedirect);
 }
 
 void InspectorPageAgent::navigate(ErrorString*, const String& url, String* outFrameId)