Reload Lo-Fi images inline instead of reloading the whole page

Reload Lo-Fi images inline instead of reloading the whole page

For all ImageResource's in the document resources list that
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/3f59410718b10864ebb27cc1e73ce98afc7ebc08
Cr-Commit-Position: refs/heads/master@{#373657}

[megjablon, 2016-01-23 01:57:27]

Description was changed from

==========
Reload Lo-Fi images inline rather than by reloading the whole page

BUG=506252
==========

to

==========
Reload Lo-Fi images inline rather than by reloading the whole page

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[megjablon, 2016-01-23 02:00:48]

Description was changed from

==========
Reload Lo-Fi images inline rather than by reloading the whole page

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reload Lo-Fi images inline instead of reloading the whole page

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[megjablon, 2016-01-23 02:06:55]

Description was changed from

==========
Reload Lo-Fi images inline instead of reloading the whole page

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reload Lo-Fi images inline instead of reloading the whole page

For all image resources in the document resources list that 
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[megjablon, 2016-01-23 02:07:00]

Patchset #1 (id:1) has been deleted

[megjablon, 2016-01-23 02:13:11]

megjablon@chromium.org changed reviewers:
+ esprehn@chromium.org

[megjablon, 2016-01-23 02:13:11]

PTAL, thanks!

esprehn: third_party/*

[megjablon, 2016-01-26 19:19:15]

megjablon@chromium.org changed reviewers:
- esprehn@chromium.org

[megjablon, 2016-01-26 19:21:08]

megjablon@chromium.org changed reviewers:
+ japhet@chromium.org

[megjablon, 2016-01-26 19:21:09]

esprehn seems busy. japhet, can you take a look at third_party/*

thanks!

[megjablon, 2016-01-27 02:35:08]

Patchset #3 (id:60001) has been deleted

[Nate Chapin, 2016-01-27 23:52:32]

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1067: if (resource &&
resource->isImage() &&
resource->response().httpHeaderField("chrome-proxy").contains("q=low")) {
It seems like most of this should live in ImageResource (everything after
determining whether it is an image and upcasting, that is).
ImageResource::reloadIfLofi() or similar?

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1069:
imageResource->mutableResourceRequest().setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache);
mutableResourceRequest() just got deleted, which strengths the argument for
putting this logic in ImageResource:
https://chromium.googlesource.com/chromium/src/+/b6ec3d48455802f26e94a913654e...

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1071:
imageResource->error(Resource::LoadError);
Why an error here?

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1072:
imageResource->load(this, defaultResourceOptions());
Does this work if the resource hasn't finished loading?

[megjablon, 2016-01-28 00:44:56]

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1067: if (resource &&
resource->isImage() &&
resource->response().httpHeaderField("chrome-proxy").contains("q=low")) {
On 2016/01/27 23:52:32, Nate Chapin wrote:
> It seems like most of this should live in ImageResource (everything after
> determining whether it is an image and upcasting, that is).
> ImageResource::reloadIfLofi() or similar?

Done.

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1069:
imageResource->mutableResourceRequest().setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache);
On 2016/01/27 23:52:32, Nate Chapin wrote:
> mutableResourceRequest() just got deleted, which strengths the argument for
> putting this logic in ImageResource:
>
https://chromium.googlesource.com/chromium/src/+/b6ec3d48455802f26e94a913654e...

Done.

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1071:
imageResource->error(Resource::LoadError);
On 2016/01/27 23:52:32, Nate Chapin wrote:
> Why an error here?

Using an error properly cancels the loading, clears the data, and notifies the
observers of an image change. I can duplicate most of this code minus the error
part since we're in ImageResource now and it does seem unintuitive to have an
error. This was the only way I could get it working with what was public from
ImageResource. Initially, I played around for awhile just using clear on the
ImageResource and Resource's m_data, but kept having issues with the
SharedBuffer lock.

https://codereview.chromium.org/1624583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1072:
imageResource->load(this, defaultResourceOptions());
On 2016/01/27 23:52:32, Nate Chapin wrote:
> Does this work if the resource hasn't finished loading?

Yes this load works as long as we wipe all previous data which is what error()
is currently doing for me.

[megjablon, 2016-01-28 00:45:20]

Description was changed from

==========
Reload Lo-Fi images inline instead of reloading the whole page

For all image resources in the document resources list that 
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reload Lo-Fi images inline instead of reloading the whole page

For all ImageResource's in the document resources list that 
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Nate Chapin, 2016-01-29 00:42:26]

LGTM

Starting a second load() on a given Resource sets off alarm bells in my head,
but I think that's because of how things used to work, not because of the way
things are now. :)

https://codereview.chromium.org/1624583003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1624583003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1069:
imageResource->reloadIfLoFi(this);
Is there any possibility that we'll someday have LoFi version of Resources other
than ImageResources? I'm wondering whether reloadIfLofi() should be am empty
virtual on Resource.

[megjablon, 2016-01-29 20:24:29]

megjablon@chromium.org changed reviewers:
+ creis@chromium.org, kenrb@chromium.org, newt@chromium.org

[megjablon, 2016-01-29 20:24:30]

kenrb: content/common/frame_messages.h
creis: content/*
newt: chrome/*

[megjablon, 2016-01-29 20:29:40]

https://codereview.chromium.org/1624583003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1624583003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1069:
imageResource->reloadIfLoFi(this);
On 2016/01/29 00:42:26, Nate Chapin wrote:
> Is there any possibility that we'll someday have LoFi version of Resources
other
> than ImageResources? I'm wondering whether reloadIfLofi() should be am empty
> virtual on Resource.

I don't think so, but we can always make it an empty virtual if we do decide to
make a LoFiResource.

[kenrb, 2016-01-29 20:54:15]

ipc lgtm

[newt (away), 2016-01-29 21:03:18]

chrome/android lgtm

[Charlie Reis, 2016-01-29 23:01:12]

Seems generally ok, but I have some questions about state and the unrelated
change to didCommit.

https://codereview.chromium.org/1624583003/diff/120001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1624583003/diff/120001/content/public/browser...
content/public/browser/web_contents.h:450: // reloads from the network.
Is this supposed to disable LoFi state as well?  Is there any LoFi state in the
browser, or does that only exist in RenderFrameImpl?

https://codereview.chromium.org/1624583003/diff/120001/content/public/browser...
content/public/browser/web_contents.h:451: virtual void ReloadLoFiImages() = 0;
This doesn't really belong in the Editing commands section.

There's no LoFi mentions in this class yet and it's kind of similar to
NavigationController::ReloadDisableLoFi, so that would be one option.

I suppose it's not really navigation related, though, so maybe it's ok to put it
below ReloadFocusedFrame in the Commands section above.  (Please reorder in the
impl files as well.)

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1651: is_using_lofi_ = false;
This line is surprising to me, since this CL isn't changing any other LoFi state
(e.g., in the browser process).  Is there no other state to update?  Or will
this put us into an inconsistent state?

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3094: if (is_main_frame_ &&
!navigation_state->WasWithinSamePage()) {
This looks unrelated to the rest of the CL, and it's subtle.  I would suggest
landing it separately (with a test) in case there are any regressions, to help
with bisecting.

By "it's subtle," I mean that we're changing from assuming the extra_data is
correct about the mode to enforcing the mode can only change for cross-document
main frame navigations, such that children inherit the mode and can't change. 
(Well, until this new reload images feature, anyway.)

Was there a bug that prompted this?

[megjablon, 2016-01-29 23:22:24]

https://chromiumcodereview.appspot.com/1624583003/diff/120001/content/public/...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/1624583003/diff/120001/content/public/...
content/public/browser/web_contents.h:450: // reloads from the network.
On 2016/01/29 23:01:12, Charlie Reis wrote:
> Is this supposed to disable LoFi state as well?  Is there any LoFi state in
the
> browser, or does that only exist in RenderFrameImpl?

Yes it disables LoFi. If the user requests that LoFi images are loaded, they
should not get more LoFi images after this request. Does
ReloadAndDisableLoFiImages work better? The only LoFi state is RenderFrameImpl.

https://chromiumcodereview.appspot.com/1624583003/diff/120001/content/public/...
content/public/browser/web_contents.h:451: virtual void ReloadLoFiImages() = 0;
On 2016/01/29 23:01:12, Charlie Reis wrote:
> This doesn't really belong in the Editing commands section.
> 
> There's no LoFi mentions in this class yet and it's kind of similar to
> NavigationController::ReloadDisableLoFi, so that would be one option.
> 
> I suppose it's not really navigation related, though, so maybe it's ok to put
it
> below ReloadFocusedFrame in the Commands section above.  (Please reorder in
the
> impl files as well.)

Done.

https://chromiumcodereview.appspot.com/1624583003/diff/120001/content/rendere...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/1624583003/diff/120001/content/rendere...
content/renderer/render_frame_impl.cc:1651: is_using_lofi_ = false;
On 2016/01/29 23:01:12, Charlie Reis wrote:
> This line is surprising to me, since this CL isn't changing any other LoFi
state
> (e.g., in the browser process).  Is there no other state to update?  Or will
> this put us into an inconsistent state?

There is no other state to update. This is the only LoFi state we store, we do
not keep the state in the browser process. I can update the method name to
OnReloadAndDisableLoFiImages if that is more clear.

https://chromiumcodereview.appspot.com/1624583003/diff/120001/content/rendere...
content/renderer/render_frame_impl.cc:3094: if (is_main_frame_ &&
!navigation_state->WasWithinSamePage()) {
On 2016/01/29 23:01:12, Charlie Reis wrote:
> This looks unrelated to the rest of the CL, and it's subtle.  I would suggest
> landing it separately (with a test) in case there are any regressions, to help
> with bisecting.
> 
> By "it's subtle," I mean that we're changing from assuming the extra_data is
> correct about the mode to enforcing the mode can only change for
cross-document
> main frame navigations, such that children inherit the mode and can't change. 
> (Well, until this new reload images feature, anyway.)
> 
> Was there a bug that prompted this?

The initial intention was this behavior. Children should only inherit the mode
and not change unless explicitly told to (such as by this reload images
feature). This wasn't visible before because reloads simply reloaded the whole
page bypassing the cache and only the main frame got the lo_fi extra data flag,
but now we need to make sure all the frames for a page have the same state and
don't switch unless a new navigation outside of the page happens.

[Charlie Reis, 2016-01-29 23:52:00]

Thanks.

https://codereview.chromium.org/1624583003/diff/120001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1624583003/diff/120001/content/public/browser...
content/public/browser/web_contents.h:450: // reloads from the network.
On 2016/01/29 23:22:23, megjablon wrote:
> On 2016/01/29 23:01:12, Charlie Reis wrote:
> > Is this supposed to disable LoFi state as well?  Is there any LoFi state in
> the
> > browser, or does that only exist in RenderFrameImpl?
> 
> Yes it disables LoFi. If the user requests that LoFi images are loaded, they
> should not get more LoFi images after this request. Does
> ReloadAndDisableLoFiImages work better? The only LoFi state is
RenderFrameImpl.

Ok.  I just wasn't clear whether there was additional state.

No need to change the method name, but please do mention that this disables LoFi
in the comment.

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1651: is_using_lofi_ = false;
On 2016/01/29 23:22:23, megjablon wrote:
> On 2016/01/29 23:01:12, Charlie Reis wrote:
> > This line is surprising to me, since this CL isn't changing any other LoFi
> state
> > (e.g., in the browser process).  Is there no other state to update?  Or will
> > this put us into an inconsistent state?
> 
> There is no other state to update. This is the only LoFi state we store, we do
> not keep the state in the browser process. I can update the method name to
> OnReloadAndDisableLoFiImages if that is more clear.

As in web_contents.h, a comment (perhaps in frame_messages.h) that this also
disables LoFi should be sufficient.

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3094: if (is_main_frame_ &&
!navigation_state->WasWithinSamePage()) {
On 2016/01/29 23:22:23, megjablon wrote:
> On 2016/01/29 23:01:12, Charlie Reis wrote:
> > This looks unrelated to the rest of the CL, and it's subtle.  I would
suggest
> > landing it separately (with a test) in case there are any regressions, to
help
> > with bisecting.
> > 
> > By "it's subtle," I mean that we're changing from assuming the extra_data is
> > correct about the mode to enforcing the mode can only change for
> cross-document
> > main frame navigations, such that children inherit the mode and can't
change. 
> > (Well, until this new reload images feature, anyway.)
> > 
> > Was there a bug that prompted this?
> 
> The initial intention was this behavior. Children should only inherit the mode
> and not change unless explicitly told to (such as by this reload images
> feature). This wasn't visible before because reloads simply reloaded the whole
> page bypassing the cache and only the main frame got the lo_fi extra data
flag,
> but now we need to make sure all the frames for a page have the same state and
> don't switch unless a new navigation outside of the page happens.

Sorry, I'm not following.

If I understand correctly, the old behavior was that main frames would set
is_using_lofi_ based on the extra_data at commit time.  Subframes would inherit
is_using_lofi_ from the main frame.

Technically, the subframes would also update their is_using_lofi_ on each
commit, but that apparently never changed the value from what was inherited by
the parent, since it was always stamped on the extra_data in willSendRequest. 
Or is this where the bug was?

I'm taking a guess-- did in-page navigations lack the extra_data bit and make a
frame (either subframe or main frame) forget that it was in LoFi mode?  That
would explain the need for this change.

Sorry for the extra questions here, but I want to understand why this change is
needed.  An example would help.

[megjablon, 2016-01-30 08:30:09]

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3094: if (is_main_frame_ &&
!navigation_state->WasWithinSamePage()) {
On 2016/01/29 23:52:00, Charlie Reis wrote:
> On 2016/01/29 23:22:23, megjablon wrote:
> > On 2016/01/29 23:01:12, Charlie Reis wrote:
> > > This looks unrelated to the rest of the CL, and it's subtle.  I would
> suggest
> > > landing it separately (with a test) in case there are any regressions, to
> help
> > > with bisecting.
> > > 
> > > By "it's subtle," I mean that we're changing from assuming the extra_data
is
> > > correct about the mode to enforcing the mode can only change for
> > cross-document
> > > main frame navigations, such that children inherit the mode and can't
> change. 
> > > (Well, until this new reload images feature, anyway.)
> > > 
> > > Was there a bug that prompted this?
> > 
> > The initial intention was this behavior. Children should only inherit the
mode
> > and not change unless explicitly told to (such as by this reload images
> > feature). This wasn't visible before because reloads simply reloaded the
whole
> > page bypassing the cache and only the main frame got the lo_fi extra data
> flag,
> > but now we need to make sure all the frames for a page have the same state
and
> > don't switch unless a new navigation outside of the page happens.
> 
> Sorry, I'm not following.
> 
> If I understand correctly, the old behavior was that main frames would set
> is_using_lofi_ based on the extra_data at commit time.  Subframes would
inherit
> is_using_lofi_ from the main frame.
> 
> Technically, the subframes would also update their is_using_lofi_ on each
> commit, but that apparently never changed the value from what was inherited by
> the parent, since it was always stamped on the extra_data in willSendRequest. 
> Or is this where the bug was?
> 
> I'm taking a guess-- did in-page navigations lack the extra_data bit and make
a
> frame (either subframe or main frame) forget that it was in LoFi mode?  That
> would explain the need for this change.
> 
> Sorry for the extra questions here, but I want to understand why this change
is
> needed.  An example would help.

The first paragraph you're describing is the only behavior I wanted. The
"technically" portion is the behavior that was unintended; I didn't want
subframes to update their value on each commit. There's no reason a subframe
should update its own LoFi state, each subframe state should match the main
frame state and only change when the main frame state does. This didn't have an
effect until now since a page would only change state with a full reload. 

I'll have to double check on this (I'm OOO till Wednesday), but what I believe
is happening is that extra_data doesn't get updated with the new LoFi state when
we turn LoFi off for a frame. Then the frame ends up turning LoFi back on.
Rather than the frame forgetting it was in LoFi mode without this extra line, I
was having the opposite issue of a frame turning LoFi back on after I had turned
it off during in-page navigations.

[Charlie Reis, 2016-02-01 22:12:00]

Thanks for the explanations.  LGTM with the note below.

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3094: if (is_main_frame_ &&
!navigation_state->WasWithinSamePage()) {
On 2016/01/30 08:30:09, megjablon wrote:
> On 2016/01/29 23:52:00, Charlie Reis wrote:
> > On 2016/01/29 23:22:23, megjablon wrote:
> > > On 2016/01/29 23:01:12, Charlie Reis wrote:
> > > > This looks unrelated to the rest of the CL, and it's subtle.  I would
> > suggest
> > > > landing it separately (with a test) in case there are any regressions,
to
> > help
> > > > with bisecting.
> > > > 
> > > > By "it's subtle," I mean that we're changing from assuming the
extra_data
> is
> > > > correct about the mode to enforcing the mode can only change for
> > > cross-document
> > > > main frame navigations, such that children inherit the mode and can't
> > change. 
> > > > (Well, until this new reload images feature, anyway.)
> > > > 
> > > > Was there a bug that prompted this?
> > > 
> > > The initial intention was this behavior. Children should only inherit the
> mode
> > > and not change unless explicitly told to (such as by this reload images
> > > feature). This wasn't visible before because reloads simply reloaded the
> whole
> > > page bypassing the cache and only the main frame got the lo_fi extra data
> > flag,
> > > but now we need to make sure all the frames for a page have the same state
> and
> > > don't switch unless a new navigation outside of the page happens.
> > 
> > Sorry, I'm not following.
> > 
> > If I understand correctly, the old behavior was that main frames would set
> > is_using_lofi_ based on the extra_data at commit time.  Subframes would
> inherit
> > is_using_lofi_ from the main frame.
> > 
> > Technically, the subframes would also update their is_using_lofi_ on each
> > commit, but that apparently never changed the value from what was inherited
by
> > the parent, since it was always stamped on the extra_data in
willSendRequest. 
> > Or is this where the bug was?
> > 
> > I'm taking a guess-- did in-page navigations lack the extra_data bit and
make
> a
> > frame (either subframe or main frame) forget that it was in LoFi mode?  That
> > would explain the need for this change.
> > 
> > Sorry for the extra questions here, but I want to understand why this change
> is
> > needed.  An example would help.
> 
> The first paragraph you're describing is the only behavior I wanted. The
> "technically" portion is the behavior that was unintended; I didn't want
> subframes to update their value on each commit. There's no reason a subframe
> should update its own LoFi state, each subframe state should match the main
> frame state and only change when the main frame state does. This didn't have
an
> effect until now since a page would only change state with a full reload. 
> 
> I'll have to double check on this (I'm OOO till Wednesday), but what I believe
> is happening is that extra_data doesn't get updated with the new LoFi state
when
> we turn LoFi off for a frame. Then the frame ends up turning LoFi back on.
> Rather than the frame forgetting it was in LoFi mode without this extra line,
I
> was having the opposite issue of a frame turning LoFi back on after I had
turned
> it off during in-page navigations.

Ok, thanks.  I'd recommend adding a test for that subframe behavior if you can,
to prevent it from regressing.

(Actually, a comment here might help as well.  Something like, "Only update LoFi
state for new main frame documents.  Subframes inherit from the main frame and
should not change at commit time.")

[megjablon, 2016-02-04 02:37:26]

@creis Added test in render_frame_impl_browsertest.cc. PTAL

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1624583003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3094: if (is_main_frame_ &&
!navigation_state->WasWithinSamePage()) {
On 2016/02/01 22:12:00, Charlie Reis wrote:
> On 2016/01/30 08:30:09, megjablon wrote:
> > On 2016/01/29 23:52:00, Charlie Reis wrote:
> > > On 2016/01/29 23:22:23, megjablon wrote:
> > > > On 2016/01/29 23:01:12, Charlie Reis wrote:
> > > > > This looks unrelated to the rest of the CL, and it's subtle.  I would
> > > suggest
> > > > > landing it separately (with a test) in case there are any regressions,
> to
> > > help
> > > > > with bisecting.
> > > > > 
> > > > > By "it's subtle," I mean that we're changing from assuming the
> extra_data
> > is
> > > > > correct about the mode to enforcing the mode can only change for
> > > > cross-document
> > > > > main frame navigations, such that children inherit the mode and can't
> > > change. 
> > > > > (Well, until this new reload images feature, anyway.)
> > > > > 
> > > > > Was there a bug that prompted this?
> > > > 
> > > > The initial intention was this behavior. Children should only inherit
the
> > mode
> > > > and not change unless explicitly told to (such as by this reload images
> > > > feature). This wasn't visible before because reloads simply reloaded the
> > whole
> > > > page bypassing the cache and only the main frame got the lo_fi extra
data
> > > flag,
> > > > but now we need to make sure all the frames for a page have the same
state
> > and
> > > > don't switch unless a new navigation outside of the page happens.
> > > 
> > > Sorry, I'm not following.
> > > 
> > > If I understand correctly, the old behavior was that main frames would set
> > > is_using_lofi_ based on the extra_data at commit time.  Subframes would
> > inherit
> > > is_using_lofi_ from the main frame.
> > > 
> > > Technically, the subframes would also update their is_using_lofi_ on each
> > > commit, but that apparently never changed the value from what was
inherited
> by
> > > the parent, since it was always stamped on the extra_data in
> willSendRequest. 
> > > Or is this where the bug was?
> > > 
> > > I'm taking a guess-- did in-page navigations lack the extra_data bit and
> make
> > a
> > > frame (either subframe or main frame) forget that it was in LoFi mode? 
That
> > > would explain the need for this change.
> > > 
> > > Sorry for the extra questions here, but I want to understand why this
change
> > is
> > > needed.  An example would help.
> > 
> > The first paragraph you're describing is the only behavior I wanted. The
> > "technically" portion is the behavior that was unintended; I didn't want
> > subframes to update their value on each commit. There's no reason a subframe
> > should update its own LoFi state, each subframe state should match the main
> > frame state and only change when the main frame state does. This didn't have
> an
> > effect until now since a page would only change state with a full reload. 
> > 
> > I'll have to double check on this (I'm OOO till Wednesday), but what I
believe
> > is happening is that extra_data doesn't get updated with the new LoFi state
> when
> > we turn LoFi off for a frame. Then the frame ends up turning LoFi back on.
> > Rather than the frame forgetting it was in LoFi mode without this extra
line,
> I
> > was having the opposite issue of a frame turning LoFi back on after I had
> turned
> > it off during in-page navigations.
> 
> Ok, thanks.  I'd recommend adding a test for that subframe behavior if you
can,
> to prevent it from regressing.
> 
> (Actually, a comment here might help as well.  Something like, "Only update
LoFi
> state for new main frame documents.  Subframes inherit from the main frame and
> should not change at commit time.")

Done.

[megjablon, 2016-02-04 19:09:57]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2016-02-04 19:09:58]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
kenrb@chromium.org, newt@chromium.org, creis@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1624583003/#ps180001 (title: "Added
comment and test")

[commit-bot: I haz the power, 2016-02-04 19:11:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1624583003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1624583003/180001

[Charlie Reis, 2016-02-04 19:13:05]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-02-04 19:30:30]

Thanks for the test!  I think it's worth covering the subframe cross-document
navigation case as well, and a few small nits otherwise.  LGTM after that (and
feel free to land once those are fixed).

https://codereview.chromium.org/1624583003/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://codereview.chromium.org/1624583003/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:43:
EXPECT_TRUE(static_cast<RenderFrameImpl*>(view_->GetMainRenderFrame())
nit: Let's use the new helper here.

https://codereview.chromium.org/1624583003/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:81: RenderFrameImpl*
main_frame() {
Style nit: GetMainRenderFrame()

(hacker_case is for trivial methods that can generally be inlined, but this
calls a virtual method that already CamelCase.)

https://codereview.chromium.org/1624583003/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:209:
EXPECT_TRUE(main_frame()->IsUsingLoFi());
Can you add a test for the subframe doing a didCommitProvisionalLoad, ensuring
that it stays in LoFi mode?

https://codereview.chromium.org/1624583003/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:217:
main_frame()->didCommitProvisionalLoad(main_frame()->GetWebFrame(), item,
It's a little surprising to me that these work with blank HistoryItems, but
that's fine.

It's also a little unexpected that this test deletes the subframe, since there
are pointers to it in the test class above.  I suppose it's safe for now since
we don't try to access it in TearDown, but maybe it's worth a comment saying
that the subframe is destroyed after this.

[megjablon, 2016-02-04 20:02:32]

https://chromiumcodereview.appspot.com/1624583003/diff/180001/content/rendere...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1624583003/diff/180001/content/rendere...
content/renderer/render_frame_impl_browsertest.cc:43:
EXPECT_TRUE(static_cast<RenderFrameImpl*>(view_->GetMainRenderFrame())
On 2016/02/04 19:30:30, Charlie Reis wrote:
> nit: Let's use the new helper here.

Done.

https://chromiumcodereview.appspot.com/1624583003/diff/180001/content/rendere...
content/renderer/render_frame_impl_browsertest.cc:81: RenderFrameImpl*
main_frame() {
On 2016/02/04 19:30:30, Charlie Reis wrote:
> Style nit: GetMainRenderFrame()
> 
> (hacker_case is for trivial methods that can generally be inlined, but this
> calls a virtual method that already CamelCase.)

Done.

https://chromiumcodereview.appspot.com/1624583003/diff/180001/content/rendere...
content/renderer/render_frame_impl_browsertest.cc:209:
EXPECT_TRUE(main_frame()->IsUsingLoFi());
On 2016/02/04 19:30:30, Charlie Reis wrote:
> Can you add a test for the subframe doing a didCommitProvisionalLoad, ensuring
> that it stays in LoFi mode?

Done.

https://chromiumcodereview.appspot.com/1624583003/diff/180001/content/rendere...
content/renderer/render_frame_impl_browsertest.cc:217:
main_frame()->didCommitProvisionalLoad(main_frame()->GetWebFrame(), item,
On 2016/02/04 19:30:30, Charlie Reis wrote:
> It's a little surprising to me that these work with blank HistoryItems, but
> that's fine.
> 
> It's also a little unexpected that this test deletes the subframe, since there
> are pointers to it in the test class above.  I suppose it's safe for now since
> we don't try to access it in TearDown, but maybe it's worth a comment saying
> that the subframe is destroyed after this.

Done. Comment added as discussed offline.

[Charlie Reis, 2016-02-04 20:08:35]

https://codereview.chromium.org/1624583003/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://codereview.chromium.org/1624583003/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:231:
EXPECT_TRUE(frame()->IsUsingLoFi());
This last line looks like the wrong behavior at first glance, and it's kind of
enforcing the incomplete navigation behavior (which isn't useful for the code
and might be a hurdle if someone wants to turn these into real navigations
later).

I'd suggest leaving this last EXPECT out and replacing with:

// The subframe would be deleted here after a cross-document navigation. It
happens to be left around in this test because this does not simulate the frame
detach.

[megjablon, 2016-02-04 20:48:09]

https://chromiumcodereview.appspot.com/1624583003/diff/200001/content/rendere...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1624583003/diff/200001/content/rendere...
content/renderer/render_frame_impl_browsertest.cc:231:
EXPECT_TRUE(frame()->IsUsingLoFi());
On 2016/02/04 20:08:35, Charlie Reis wrote:
> This last line looks like the wrong behavior at first glance, and it's kind of
> enforcing the incomplete navigation behavior (which isn't useful for the code
> and might be a hurdle if someone wants to turn these into real navigations
> later).
> 
> I'd suggest leaving this last EXPECT out and replacing with:
> 
> // The subframe would be deleted here after a cross-document navigation. It
> happens to be left around in this test because this does not simulate the
frame
> detach.

Done.

[Charlie Reis, 2016-02-04 21:21:22]

Thanks, LGTM.

[megjablon, 2016-02-04 21:36:25]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2016-02-04 21:36:26]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
japhet@chromium.org, newt@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1624583003/#ps220001 (title: "added
comment")

[commit-bot: I haz the power, 2016-02-04 21:38:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1624583003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1624583003/220001

[commit-bot: I haz the power, 2016-02-04 23:28:02]

Description was changed from

==========
Reload Lo-Fi images inline instead of reloading the whole page

For all ImageResource's in the document resources list that 
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reload Lo-Fi images inline instead of reloading the whole page

For all ImageResource's in the document resources list that 
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-02-04 23:28:03]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-02-04 23:29:06]

Description was changed from

==========
Reload Lo-Fi images inline instead of reloading the whole page

For all ImageResource's in the document resources list that 
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reload Lo-Fi images inline instead of reloading the whole page

For all ImageResource's in the document resources list that
have "q=low" directives in the Chrome-Proxy response header,
clear the old image data and reload with Lo-Fi disabled.

BUG=506252
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/3f59410718b10864ebb27cc1e73ce98afc7ebc08
Cr-Commit-Position: refs/heads/master@{#373657}
==========

[commit-bot: I haz the power, 2016-02-04 23:29:07]

Patchset 10 (id:??) landed as
https://crrev.com/3f59410718b10864ebb27cc1e73ce98afc7ebc08
Cr-Commit-Position: refs/heads/master@{#373657}