Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1112513005:
Reload image bypassing the cache (Closed)
|
Created:
5 years, 7 months ago by megjablon Modified:
5 years, 5 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, groby+blinkspell_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionReload image bypassing the cache.
Provide a way to reload an image node, bypassing the cache,
to load an original image in place. Also, give the context
menu the request's extra data for images.
BUG=485295
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195008
Patch Set 1 #
Total comments: 11
Patch Set 2 : WebNode instead of WebPoint #
Total comments: 6
Patch Set 3 : forceReload and remove bypassCache #Patch Set 4 : Using ExtraData #
Total comments: 12
Patch Set 5 : japhet comments #
Total comments: 1
Patch Set 6 : nit #
Total comments: 3
Messages
Total messages: 39 (8 generated)
bengr@chromium.org changed reviewers: + bengr@chromium.org
https://codereview.chromium.org/1112513005/diff/1/Source/web/ContextMenuClien... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/web/ContextMenuClien... Source/web/ContextMenuClientImpl.cpp:239: data.imageChromeProxyResponseHeader = imageElement->imageLoader().image()->response().httpHeaderField("via").utf8().data(); There shouldn't be anything DRP-specific in this CL.
yoav@yoav.ws changed reviewers: + yoav@yoav.ws
Can you please explain the logic behind this menu item and what it should do? Also - Is there a way we can test that this is actually working? What do we usually do for menu items? https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... Source/core/loader/ImageLoader.cpp:103: m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior, false); This shouldn't be a boolean, but an enum. That way we can tell what it does from when calling the method. Also - is the bypassCache state always false for images that are asynchronously loaded? Can you explain the logic?
On 2015/04/29 06:13:24, Yoav Weiss wrote: > Can you please explain the logic behind this menu item and what it should do? > Specifically, what are LoFi images?
japhet@chromium.org changed reviewers: + japhet@chromium.org
yoav: megjablon sent me a link to https://docs.google.com/a/chromium.org/document/d/1v8F7SjFJJ0aGql3lWHfbVwUwcg..., which should be visible with a @chromium.org account.
On 2015/04/30 20:33:56, Nate Chapin wrote: > yoav: megjablon sent me a link to > https://docs.google.com/a/chromium.org/document/d/1v8F7SjFJJ0aGql3lWHfbVwUwcg..., > which should be visible with a @chromium.org account. I don't have a chromium account :) I requested access but it would have been great if document was public...
https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.cpp File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.... Source/core/editing/Editor.cpp:973: void Editor::reloadImage(const HitTestResult& result, bool bypassCache) This doesn't really have anything to do with Editor, so it shouldn't be plumbed through there. https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... Source/core/loader/ImageLoader.cpp:269: resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); I'm not thrilled with the need to bypass the cache when loading the real image contents. That seems like it could break if LoFi images are enabled, then disabled. Perhaps LoFi images could include Cache-Control: headers? https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:3556: HitTestResult result = hitTestResultForViewportPos(point); The need to do a hit test to re-find the element is probably expensive and possibly flaky. RenderFrameImpl::context_menu_node_ stores the WebNode that spawned the context menu. Perhaps, when the menu closes, we could use that to find the right element?
https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.cpp File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.... Source/core/editing/Editor.cpp:973: void Editor::reloadImage(const HitTestResult& result, bool bypassCache) On 2015/04/30 20:59:41, Nate Chapin wrote: > This doesn't really have anything to do with Editor, so it shouldn't be plumbed > through there. Do you have any ideas for an alternate path? I was following the copyImage path. https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... Source/core/loader/ImageLoader.cpp:269: resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); On 2015/04/30 20:59:41, Nate Chapin wrote: > I'm not thrilled with the need to bypass the cache when loading the real image > contents. That seems like it could break if LoFi images are enabled, then > disabled. Perhaps LoFi images could include Cache-Control: headers? We discussed using no-store, but want LoFi images to be cached since the goal of Data Saver is to be conservative with users' data. We want to bypass the cache and load the original image only when the user explicitly asks to. https://codereview.chromium.org/1112513005/diff/1/Source/web/ContextMenuClien... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/web/ContextMenuClien... Source/web/ContextMenuClientImpl.cpp:239: data.imageChromeProxyResponseHeader = imageElement->imageLoader().image()->response().httpHeaderField("via").utf8().data(); On 2015/04/29 00:23:21, bengr wrote: > There shouldn't be anything DRP-specific in this CL. investigating https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:3556: HitTestResult result = hitTestResultForViewportPos(point); On 2015/04/30 20:59:41, Nate Chapin wrote: > The need to do a hit test to re-find the element is probably expensive and > possibly flaky. RenderFrameImpl::context_menu_node_ stores the WebNode that > spawned the context menu. Perhaps, when the menu closes, we could use that to > find the right element? You you point me to the code for this? This follows the path of the save image and copy image context menu options.
On 2015/04/30 22:00:55, megjablon wrote: > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.cpp > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.... > Source/core/editing/Editor.cpp:973: void Editor::reloadImage(const > HitTestResult& result, bool bypassCache) > On 2015/04/30 20:59:41, Nate Chapin wrote: > > This doesn't really have anything to do with Editor, so it shouldn't be > plumbed > > through there. > > Do you have any ideas for an alternate path? I was following the copyImage path. > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp#... > Source/web/WebViewImpl.cpp:3556: HitTestResult result = > hitTestResultForViewportPos(point); > On 2015/04/30 20:59:41, Nate Chapin wrote: > > The need to do a hit test to re-find the element is probably expensive and > > possibly flaky. RenderFrameImpl::context_menu_node_ stores the WebNode that > > spawned the context menu. Perhaps, when the menu closes, we could use that to > > find the right element? > > You you point me to the code for this? This follows the path of the save image > and copy image context menu options. Ideally, whatever api blink provides in public/web/ will take a WebNode (or some subclass of WebNode) instead of a WebPoint. We should be about to use context_menu_node_ in chromim's content/renderer/render_frame_impl.cc to get that WebNode. Re: Editor, it is related to a fairly specific set of tools for modifying the content of a web page, and reloading an image doesn't really fit in that category. If we have a WebNode in the reloadImage callback, we can skip Editor entirely. We can just unwrap the WebNode, ensure it is actually an HTMLImageElement, and use the HTMLImageElement right there.
https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... Source/core/loader/ImageLoader.cpp:269: resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); On 2015/04/30 22:00:54, megjablon wrote: > On 2015/04/30 20:59:41, Nate Chapin wrote: > > I'm not thrilled with the need to bypass the cache when loading the real image > > contents. That seems like it could break if LoFi images are enabled, then > > disabled. Perhaps LoFi images could include Cache-Control: headers? > > We discussed using no-store, but want LoFi images to be cached since the goal of > Data Saver is to be conservative with users' data. We want to bypass the cache > and load the original image only when the user explicitly asks to. LoFi images would be replaced with the real image one-at-a-time, right? ResourceFetcher already has a setting to auto load images, I wonder if we could modify that to handle this use case.
https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... Source/core/loader/ImageLoader.cpp:269: resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); On 2015/04/30 22:26:54, Nate Chapin wrote: > On 2015/04/30 22:00:54, megjablon wrote: > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > I'm not thrilled with the need to bypass the cache when loading the real > image > > > contents. That seems like it could break if LoFi images are enabled, then > > > disabled. Perhaps LoFi images could include Cache-Control: headers? > > > > We discussed using no-store, but want LoFi images to be cached since the goal > of > > Data Saver is to be conservative with users' data. We want to bypass the cache > > and load the original image only when the user explicitly asks to. > > LoFi images would be replaced with the real image one-at-a-time, right? > ResourceFetcher already has a setting to auto load images, I wonder if we could > modify that to handle this use case. Yes, we want to only replace one real image at a time. What does auto loading an image do?
On 2015/04/30 22:35:50, megjablon wrote: > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > File Source/core/loader/ImageLoader.cpp (right): > > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > Source/core/loader/ImageLoader.cpp:269: > resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); > On 2015/04/30 22:26:54, Nate Chapin wrote: > > On 2015/04/30 22:00:54, megjablon wrote: > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > I'm not thrilled with the need to bypass the cache when loading the real > > image > > > > contents. That seems like it could break if LoFi images are enabled, then > > > > disabled. Perhaps LoFi images could include Cache-Control: headers? > > > > > > We discussed using no-store, but want LoFi images to be cached since the > goal > > of > > > Data Saver is to be conservative with users' data. We want to bypass the > cache > > > and load the original image only when the user explicitly asks to. > > > > LoFi images would be replaced with the real image one-at-a-time, right? > > ResourceFetcher already has a setting to auto load images, I wonder if we > could > > modify that to handle this use case. > > Yes, we want to only replace one real image at a time. What does auto loading an > image do? The default behavior is to auto-load images. I should have said: there's a setting to refrain from auto-loading images and instead wait for the user to ask for the images to be loaded.
On 2015/04/30 22:24:03, Nate Chapin wrote: > On 2015/04/30 22:00:55, megjablon wrote: > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.cpp > > File Source/core/editing/Editor.cpp (right): > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.... > > Source/core/editing/Editor.cpp:973: void Editor::reloadImage(const > > HitTestResult& result, bool bypassCache) > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > This doesn't really have anything to do with Editor, so it shouldn't be > > plumbed > > > through there. > > > > Do you have any ideas for an alternate path? I was following the copyImage > path. > > > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp > > File Source/web/WebViewImpl.cpp (right): > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp#... > > Source/web/WebViewImpl.cpp:3556: HitTestResult result = > > hitTestResultForViewportPos(point); > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > The need to do a hit test to re-find the element is probably expensive and > > > possibly flaky. RenderFrameImpl::context_menu_node_ stores the WebNode that > > > spawned the context menu. Perhaps, when the menu closes, we could use that > to > > > find the right element? > > > > You you point me to the code for this? This follows the path of the save image > > and copy image context menu options. > > Ideally, whatever api blink provides in public/web/ will take a WebNode (or some > subclass of WebNode) instead of a WebPoint. We should be about to use > context_menu_node_ in chromim's content/renderer/render_frame_impl.cc to get > that WebNode. > > Re: Editor, it is related to a fairly specific set of tools for modifying the > content of a web page, and reloading an image doesn't really fit in that > category. If we have a WebNode in the reloadImage callback, we can skip Editor > entirely. We can just unwrap the WebNode, ensure it is actually an > HTMLImageElement, and use the HTMLImageElement right there. How can I get from a WebNode to a Node or Element?
On 2015/04/30 22:37:49, Nate Chapin wrote: > On 2015/04/30 22:35:50, megjablon wrote: > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > > File Source/core/loader/ImageLoader.cpp (right): > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > > Source/core/loader/ImageLoader.cpp:269: > > > resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); > > On 2015/04/30 22:26:54, Nate Chapin wrote: > > > On 2015/04/30 22:00:54, megjablon wrote: > > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > > I'm not thrilled with the need to bypass the cache when loading the real > > > image > > > > > contents. That seems like it could break if LoFi images are enabled, > then > > > > > disabled. Perhaps LoFi images could include Cache-Control: headers? > > > > > > > > We discussed using no-store, but want LoFi images to be cached since the > > goal > > > of > > > > Data Saver is to be conservative with users' data. We want to bypass the > > cache > > > > and load the original image only when the user explicitly asks to. > > > > > > LoFi images would be replaced with the real image one-at-a-time, right? > > > ResourceFetcher already has a setting to auto load images, I wonder if we > > could > > > modify that to handle this use case. > > > > Yes, we want to only replace one real image at a time. What does auto loading > an > > image do? > > The default behavior is to auto-load images. I should have said: there's a > setting to refrain from auto-loading images and instead wait for the user to ask > for the images to be loaded. Can you elaborate on how this could be used to bypass the cache?
On 2015/05/01 00:35:56, megjablon wrote: > On 2015/04/30 22:24:03, Nate Chapin wrote: > > On 2015/04/30 22:00:55, megjablon wrote: > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.cpp > > > File Source/core/editing/Editor.cpp (right): > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.... > > > Source/core/editing/Editor.cpp:973: void Editor::reloadImage(const > > > HitTestResult& result, bool bypassCache) > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > This doesn't really have anything to do with Editor, so it shouldn't be > > > plumbed > > > > through there. > > > > > > Do you have any ideas for an alternate path? I was following the copyImage > > path. > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp > > > File Source/web/WebViewImpl.cpp (right): > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp#... > > > Source/web/WebViewImpl.cpp:3556: HitTestResult result = > > > hitTestResultForViewportPos(point); > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > The need to do a hit test to re-find the element is probably expensive and > > > > possibly flaky. RenderFrameImpl::context_menu_node_ stores the WebNode > that > > > > spawned the context menu. Perhaps, when the menu closes, we could use that > > to > > > > find the right element? > > > > > > You you point me to the code for this? This follows the path of the save > image > > > and copy image context menu options. > > > > Ideally, whatever api blink provides in public/web/ will take a WebNode (or > some > > subclass of WebNode) instead of a WebPoint. We should be about to use > > context_menu_node_ in chromim's content/renderer/render_frame_impl.cc to get > > that WebNode. > > > > Re: Editor, it is related to a fairly specific set of tools for modifying the > > content of a web page, and reloading an image doesn't really fit in that > > category. If we have a WebNode in the reloadImage callback, we can skip Editor > > entirely. We can just unwrap the WebNode, ensure it is actually an > > HTMLImageElement, and use the HTMLImageElement right there. > > How can I get from a WebNode to a Node or Element? If you're inside blink, there's an unwrap() function (as well as an operator) that will get the underlying Node* from a WebNode. From there, isHTMLImageElement() and toHTMLImageElement() can be used to verify that the Node is the expected subclass and safely do the cast for you.
On 2015/05/01 00:36:39, megjablon wrote: > On 2015/04/30 22:37:49, Nate Chapin wrote: > > On 2015/04/30 22:35:50, megjablon wrote: > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > > > File Source/core/loader/ImageLoader.cpp (right): > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > > > Source/core/loader/ImageLoader.cpp:269: > > > > > > resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); > > > On 2015/04/30 22:26:54, Nate Chapin wrote: > > > > On 2015/04/30 22:00:54, megjablon wrote: > > > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > > > I'm not thrilled with the need to bypass the cache when loading the > real > > > > image > > > > > > contents. That seems like it could break if LoFi images are enabled, > > then > > > > > > disabled. Perhaps LoFi images could include Cache-Control: headers? > > > > > > > > > > We discussed using no-store, but want LoFi images to be cached since the > > > goal > > > > of > > > > > Data Saver is to be conservative with users' data. We want to bypass the > > > cache > > > > > and load the original image only when the user explicitly asks to. > > > > > > > > LoFi images would be replaced with the real image one-at-a-time, right? > > > > ResourceFetcher already has a setting to auto load images, I wonder if we > > > could > > > > modify that to handle this use case. > > > > > > Yes, we want to only replace one real image at a time. What does auto > loading > > an > > > image do? > > > > The default behavior is to auto-load images. I should have said: there's a > > setting to refrain from auto-loading images and instead wait for the user to > ask > > for the images to be loaded. > Can you elaborate on how this could be used to bypass the cache? I had been wondering if, instead of having the proxy return a dummy image, we could have blink check whether we're in LoFi-mode and just populate a dummy image itself. If that were a viable solution, we could probably avoid putting the dummy image in any caches in the first place.
On 2015/05/01 17:15:08, Nate Chapin wrote: > On 2015/05/01 00:35:56, megjablon wrote: > > On 2015/04/30 22:24:03, Nate Chapin wrote: > > > On 2015/04/30 22:00:55, megjablon wrote: > > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.cpp > > > > File Source/core/editing/Editor.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/editing/Editor.... > > > > Source/core/editing/Editor.cpp:973: void Editor::reloadImage(const > > > > HitTestResult& result, bool bypassCache) > > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > > This doesn't really have anything to do with Editor, so it shouldn't be > > > > plumbed > > > > > through there. > > > > > > > > Do you have any ideas for an alternate path? I was following the copyImage > > > path. > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp > > > > File Source/web/WebViewImpl.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/web/WebViewImpl.cpp#... > > > > Source/web/WebViewImpl.cpp:3556: HitTestResult result = > > > > hitTestResultForViewportPos(point); > > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > > The need to do a hit test to re-find the element is probably expensive > and > > > > > possibly flaky. RenderFrameImpl::context_menu_node_ stores the WebNode > > that > > > > > spawned the context menu. Perhaps, when the menu closes, we could use > that > > > to > > > > > find the right element? > > > > > > > > You you point me to the code for this? This follows the path of the save > > image > > > > and copy image context menu options. > > > > > > Ideally, whatever api blink provides in public/web/ will take a WebNode (or > > some > > > subclass of WebNode) instead of a WebPoint. We should be about to use > > > context_menu_node_ in chromim's content/renderer/render_frame_impl.cc to get > > > that WebNode. > > > > > > Re: Editor, it is related to a fairly specific set of tools for modifying > the > > > content of a web page, and reloading an image doesn't really fit in that > > > category. If we have a WebNode in the reloadImage callback, we can skip > Editor > > > entirely. We can just unwrap the WebNode, ensure it is actually an > > > HTMLImageElement, and use the HTMLImageElement right there. > > > > How can I get from a WebNode to a Node or Element? > > If you're inside blink, there's an unwrap() function (as well as an operator) > that will get the underlying Node* from a WebNode. From there, > isHTMLImageElement() and toHTMLImageElement() can be used to verify that the > Node is the expected subclass and safely do the cast for you. Uploaded this change. Can you take a look?
On 2015/05/01 17:17:18, Nate Chapin wrote: > On 2015/05/01 00:36:39, megjablon wrote: > > On 2015/04/30 22:37:49, Nate Chapin wrote: > > > On 2015/04/30 22:35:50, megjablon wrote: > > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > > > > File Source/core/loader/ImageLoader.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1112513005/diff/1/Source/core/loader/ImageLoa... > > > > Source/core/loader/ImageLoader.cpp:269: > > > > > > > > > > resourceRequest.setCachePolicy(ResourceRequestCachePolicy::ReloadBypassingCache); > > > > On 2015/04/30 22:26:54, Nate Chapin wrote: > > > > > On 2015/04/30 22:00:54, megjablon wrote: > > > > > > On 2015/04/30 20:59:41, Nate Chapin wrote: > > > > > > > I'm not thrilled with the need to bypass the cache when loading the > > real > > > > > image > > > > > > > contents. That seems like it could break if LoFi images are enabled, > > > then > > > > > > > disabled. Perhaps LoFi images could include Cache-Control: headers? > > > > > > > > > > > > We discussed using no-store, but want LoFi images to be cached since > the > > > > goal > > > > > of > > > > > > Data Saver is to be conservative with users' data. We want to bypass > the > > > > cache > > > > > > and load the original image only when the user explicitly asks to. > > > > > > > > > > LoFi images would be replaced with the real image one-at-a-time, right? > > > > > ResourceFetcher already has a setting to auto load images, I wonder if > we > > > > could > > > > > modify that to handle this use case. > > > > > > > > Yes, we want to only replace one real image at a time. What does auto > > loading > > > an > > > > image do? > > > > > > The default behavior is to auto-load images. I should have said: there's a > > > setting to refrain from auto-loading images and instead wait for the user to > > ask > > > for the images to be loaded. > > Can you elaborate on how this could be used to bypass the cache? > > I had been wondering if, instead of having the proxy return a dummy image, we > could have blink check whether we're in LoFi-mode and just populate a dummy > image itself. If that were a viable solution, we could probably avoid putting > the dummy image in any caches in the first place. This was considered, but would require knowing the image dimensions.
https://codereview.chromium.org/1112513005/diff/20001/Source/core/html/HTMLIm... File Source/core/html/HTMLImageElement.h (left): https://codereview.chromium.org/1112513005/diff/20001/Source/core/html/HTMLIm... Source/core/html/HTMLImageElement.h:148: HTMLImageLoader& imageLoader() const { return *m_imageLoader; } This should probably stay protected. Maybe have a public function called forceReload() (or similar)? https://codereview.chromium.org/1112513005/diff/20001/Source/core/loader/Imag... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/1112513005/diff/20001/Source/core/loader/Imag... Source/core/loader/ImageLoader.h:84: void updateFromElement(UpdateFromElementBehavior = UpdateNormal, bool bypassCache = false); Instead of the 2nd parameter, let's add a new value to the UpdateFromElementBehavior enum. Maybe UpdateForcedReload? https://codereview.chromium.org/1112513005/diff/20001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/20001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:239: data.imageChromeProxyResponseHeader = imageElement->imageLoader().image()->response().httpHeaderField("via").utf8().data(); What is "via"? It seems wrong to be digging down into response headers here.
https://codereview.chromium.org/1112513005/diff/20001/Source/core/html/HTMLIm... File Source/core/html/HTMLImageElement.h (left): https://codereview.chromium.org/1112513005/diff/20001/Source/core/html/HTMLIm... Source/core/html/HTMLImageElement.h:148: HTMLImageLoader& imageLoader() const { return *m_imageLoader; } On 2015/05/01 18:46:51, Nate Chapin wrote: > This should probably stay protected. Maybe have a public function called > forceReload() (or similar)? Done. https://codereview.chromium.org/1112513005/diff/20001/Source/core/loader/Imag... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/1112513005/diff/20001/Source/core/loader/Imag... Source/core/loader/ImageLoader.h:84: void updateFromElement(UpdateFromElementBehavior = UpdateNormal, bool bypassCache = false); On 2015/05/01 18:46:51, Nate Chapin wrote: > Instead of the 2nd parameter, let's add a new value to the > UpdateFromElementBehavior enum. Maybe UpdateForcedReload? I like this idea. Done. https://codereview.chromium.org/1112513005/diff/20001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/20001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:239: data.imageChromeProxyResponseHeader = imageElement->imageLoader().image()->response().httpHeaderField("via").utf8().data(); On 2015/05/01 18:46:51, Nate Chapin wrote: > What is "via"? It seems wrong to be digging down into response headers here. We need a way of telling the context menu that an image was a lo-fi image. The server sends (or will send soon) the response header "chrome-proxy: q=low" for lo-fi images. I'm currently using the Data Saver via header until this becomes available. This lo-fi specific logic definitely needs to be moved out of here. Can I just surface all the response headers to the context menu?
Again, tests would be good :) I have to say that I'd be significantly more comfortable with that change if it wasn't ChromeProxy-specific. Did you try to standardize an HTTP header based "low quality image" signal? Are there any plans to standardize one in the future? Of course, every server could later use the magic "chrome-proxy: q=low" header to get the same effect, but maybe we can make it more generic and interoperable? And finally, since this is a Web exposed change, Can you send out an intent to implement and ship? (since this change is not behind a flag) On 2015/05/01 19:15:24, megjablon wrote: > https://codereview.chromium.org/1112513005/diff/20001/Source/core/html/HTMLIm... > File Source/core/html/HTMLImageElement.h (left): > > https://codereview.chromium.org/1112513005/diff/20001/Source/core/html/HTMLIm... > Source/core/html/HTMLImageElement.h:148: HTMLImageLoader& imageLoader() const { > return *m_imageLoader; } > On 2015/05/01 18:46:51, Nate Chapin wrote: > > This should probably stay protected. Maybe have a public function called > > forceReload() (or similar)? > > Done. > > https://codereview.chromium.org/1112513005/diff/20001/Source/core/loader/Imag... > File Source/core/loader/ImageLoader.h (right): > > https://codereview.chromium.org/1112513005/diff/20001/Source/core/loader/Imag... > Source/core/loader/ImageLoader.h:84: void > updateFromElement(UpdateFromElementBehavior = UpdateNormal, bool bypassCache = > false); > On 2015/05/01 18:46:51, Nate Chapin wrote: > > Instead of the 2nd parameter, let's add a new value to the > > UpdateFromElementBehavior enum. Maybe UpdateForcedReload? > > I like this idea. Done. Significantly better. Image loading part looks good. > > https://codereview.chromium.org/1112513005/diff/20001/Source/web/ContextMenuC... > File Source/web/ContextMenuClientImpl.cpp (right): > > https://codereview.chromium.org/1112513005/diff/20001/Source/web/ContextMenuC... > Source/web/ContextMenuClientImpl.cpp:239: data.imageChromeProxyResponseHeader = > imageElement->imageLoader().image()->response().httpHeaderField("via").utf8().data(); > On 2015/05/01 18:46:51, Nate Chapin wrote: > > What is "via"? It seems wrong to be digging down into response headers here. > > We need a way of telling the context menu that an image was a lo-fi image. The > server sends (or will send soon) the response header "chrome-proxy: q=low" for > lo-fi images. I'm currently using the Data Saver via header until this becomes > available. This lo-fi specific logic definitely needs to be moved out of here. > Can I just surface all the response headers to the context menu? Looking into the header response at ImageResource::responseReceived and saving it as a boolean is probably cleaner. Also, are you sure imageElement is not null? If so, ASSERT it, otherwise check it. Same for imageElement->imageLoader().image()
Almost there! Just a few cleanup items. https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... File Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... Source/core/html/HTMLImageElement.cpp:650: return 0; Nit: ResourceResponse() https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... File Source/core/html/HTMLImageElement.h (right): https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... Source/core/html/HTMLImageElement.h:114: const ResourceResponse& getResponse() const; Nit: blink style prefers not to prefix getters with "get", so this should just be "response()". https://codereview.chromium.org/1112513005/diff/60001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/60001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:239: data.imageResponseExtraData = 0; This else clause isn't necessary if this is properly initialized in the WebContextMenuData constructor. https://codereview.chromium.org/1112513005/diff/60001/public/web/WebContextMe... File public/web/WebContextMenuData.h (right): https://codereview.chromium.org/1112513005/diff/60001/public/web/WebContextMe... public/web/WebContextMenuData.h:84: WebURLResponse::ExtraData* imageResponseExtraData; Initialize this to nullptr in constructor https://codereview.chromium.org/1112513005/diff/60001/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/1112513005/diff/60001/public/web/WebView.h#ne... public/web/WebView.h:335: virtual void reloadImageAt(const WebNode&) = 0; This should probably just be reloadImage() now. It should probably also go on WebLocalFrame instead of WebView, now that it's more strongly tied to the context menu logic that lives in RenderFrameImpl.
https://codereview.chromium.org/1112513005/diff/60001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/60001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:236: if (imageElement && imageElement->getResponse()) Oh, I forgot. Since response() shouldn't return null, you'd want imageElement->response().isNull() here.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... File Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... Source/core/html/HTMLImageElement.cpp:650: return 0; On 2015/05/05 22:03:49, Nate Chapin wrote: > Nit: ResourceResponse() This can't be done because it's a local temporary object. Using cachedImage() in ContextMenuClientImpl now. https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... File Source/core/html/HTMLImageElement.h (right): https://codereview.chromium.org/1112513005/diff/60001/Source/core/html/HTMLIm... Source/core/html/HTMLImageElement.h:114: const ResourceResponse& getResponse() const; On 2015/05/05 22:03:49, Nate Chapin wrote: > Nit: blink style prefers not to prefix getters with "get", so this should just > be "response()". Done. https://codereview.chromium.org/1112513005/diff/60001/Source/web/ContextMenuC... File Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/60001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:236: if (imageElement && imageElement->getResponse()) On 2015/05/05 22:05:44, Nate Chapin wrote: > Oh, I forgot. Since response() shouldn't return null, you'd want > imageElement->response().isNull() here. Done. https://codereview.chromium.org/1112513005/diff/60001/Source/web/ContextMenuC... Source/web/ContextMenuClientImpl.cpp:239: data.imageResponseExtraData = 0; On 2015/05/05 22:03:49, Nate Chapin wrote: > This else clause isn't necessary if this is properly initialized in the > WebContextMenuData constructor. Done. https://codereview.chromium.org/1112513005/diff/60001/public/web/WebContextMe... File public/web/WebContextMenuData.h (right): https://codereview.chromium.org/1112513005/diff/60001/public/web/WebContextMe... public/web/WebContextMenuData.h:84: WebURLResponse::ExtraData* imageResponseExtraData; On 2015/05/05 22:03:49, Nate Chapin wrote: > Initialize this to nullptr in constructor Done. https://codereview.chromium.org/1112513005/diff/60001/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/1112513005/diff/60001/public/web/WebView.h#ne... public/web/WebView.h:335: virtual void reloadImageAt(const WebNode&) = 0; On 2015/05/05 22:03:49, Nate Chapin wrote: > This should probably just be reloadImage() now. It should probably also go on > WebLocalFrame instead of WebView, now that it's more strongly tied to the > context menu logic that lives in RenderFrameImpl. Done.
lgtm with one nitpick https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... File public/web/WebLocalFrame.h (right): https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... public/web/WebLocalFrame.h:139: // Reload the image located at a particular point in the WebLocalFrame (if there is such an image). This comment is out of date.
On 2015/05/06 18:40:54, Nate Chapin wrote: > lgtm with one nitpick > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > File public/web/WebLocalFrame.h (right): > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > public/web/WebLocalFrame.h:139: // Reload the image located at a particular > point in the WebLocalFrame (if there is such an image). > This comment is out of date. This still needs a way to tell the request to turn off LoFi. What issues are there with passing a header from the context menu to add to the request?
On 2015/05/06 18:43:48, megjablon wrote: > On 2015/05/06 18:40:54, Nate Chapin wrote: > > lgtm with one nitpick > > > > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > > File public/web/WebLocalFrame.h (right): > > > > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > > public/web/WebLocalFrame.h:139: // Reload the image located at a particular > > point in the WebLocalFrame (if there is such an image). > > This comment is out of date. > > This still needs a way to tell the request to turn off LoFi. What issues are > there with passing > a header from the context menu to add to the request? ReloadBypassingCache + image request isn't a sufficient hueristic I assume?
On 2015/05/06 18:45:35, Nate Chapin wrote: > On 2015/05/06 18:43:48, megjablon wrote: > > On 2015/05/06 18:40:54, Nate Chapin wrote: > > > lgtm with one nitpick > > > > > > > > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > > > File public/web/WebLocalFrame.h (right): > > > > > > > > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > > > public/web/WebLocalFrame.h:139: // Reload the image located at a particular > > > point in the WebLocalFrame (if there is such an image). > > > This comment is out of date. > > > > This still needs a way to tell the request to turn off LoFi. What issues are > > there with passing > > a header from the context menu to add to the request? > > ReloadBypassingCache + image request isn't a sufficient hueristic I assume? This will probably work almost all of the time, but I'm sure we're overlooking some edge case where it won't. That said, I'm ok with landing the CL with such a heuristic and adding a proper path in a follow-up.
On 2015/05/06 20:47:20, bengr wrote: > On 2015/05/06 18:45:35, Nate Chapin wrote: > > On 2015/05/06 18:43:48, megjablon wrote: > > > On 2015/05/06 18:40:54, Nate Chapin wrote: > > > > lgtm with one nitpick > > > > > > > > > > > > > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > > > > File public/web/WebLocalFrame.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1112513005/diff/100001/public/web/WebLocalFra... > > > > public/web/WebLocalFrame.h:139: // Reload the image located at a > particular > > > > point in the WebLocalFrame (if there is such an image). > > > > This comment is out of date. > > > > > > This still needs a way to tell the request to turn off LoFi. What issues are > > > there with passing > > > a header from the context menu to add to the request? > > > > ReloadBypassingCache + image request isn't a sufficient hueristic I assume? > > This will probably work almost all of the time, but I'm sure we're overlooking > some edge case where it won't. That said, I'm ok with landing the CL with such a > heuristic and adding a proper path in a follow-up. My best guess for a reliable solution is a similar use of ExtraData as the context menu logic uses for resposnes. Maybe that could get set in RenderFrameImpl::willSendRequest()?
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1112513005/#ps140001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1112513005/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195008
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
Can we add an API that's more specific, like WebElement::reloadLofiImage() and make blink aware of the lofi images. Having the wrong entries in the cache doesn't seem right. You want the image resources to actually be different and the fetch layer above it to select one or the other. https://codereview.chromium.org/1112513005/diff/140001/Source/web/WebLocalFra... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1112513005/diff/140001/Source/web/WebLocalFra... Source/web/WebLocalFrameImpl.cpp:927: void WebLocalFrameImpl::reloadImage(const WebNode& webNode) This should be a method on WebElement, not WebLocalFrame. https://codereview.chromium.org/1112513005/diff/140001/public/web/WebContextM... File public/web/WebContextMenuData.h (right): https://codereview.chromium.org/1112513005/diff/140001/public/web/WebContextM... public/web/WebContextMenuData.h:82: // If |media_type| is MediaTypeImage and |has_image_contents| is true, then mediaType and hasImageContents https://codereview.chromium.org/1112513005/diff/140001/public/web/WebLocalFra... File public/web/WebLocalFrame.h (right): https://codereview.chromium.org/1112513005/diff/140001/public/web/WebLocalFra... public/web/WebLocalFrame.h:140: virtual void reloadImage(const WebNode&) = 0; This should be on WebElement |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
