Bug 76895 – Adding Chromium feature to turn on encoding detector from response

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Keishi Hattori]

The second half of the patch for the encoding detector infobar feature.
https://bugs.webkit.org/show_bug.cgi?id=75594

We need to remember if users had turned on the encoding detector in the past, so we don't show the infobar too frequently.
And we decided to implement it by adding a field(needsEncodingDetection) in the resource response.

Chromium will turn the flag on for resources that need encoding detection. The WebKit side should look at that flag and temporarily turn on the encoding detector for that resource.

[Keishi Hattori]

Created attachment 123699 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 123699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123699&action=review

> Source/WebKit/chromium/public/platform/WebURLResponse.h:177
> +    WEBKIT_EXPORT bool needsEncodingDetection() const;

This feels like a layering violation.  Why does the network stack need to know
about this?  Maybe this should be a method on WebDataSource instead?  It is not
clear to me where you call setNeedsEncodingDetection, but it looks like you only
care about it when loading documents.  (WebDataSource is the API wrapper for
DocumentLoader.)

[Keishi Hattori]

Thanks for the quick response.

(In reply to comment #3)
> (From update of attachment 123699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123699&action=review
> 
> > Source/WebKit/chromium/public/platform/WebURLResponse.h:177
> > +    WEBKIT_EXPORT bool needsEncodingDetection() const;
> 
> This feels like a layering violation.  Why does the network stack need to know
> about this?

We needed to lookup the list of sites to turn on the encoding detector very fast.
And we wanted to avoid having to sync the list to all of the renderer processes.

> Maybe this should be a method on WebDataSource instead?  It is not
> clear to me where you call setNeedsEncodingDetection, but it looks like you only
> care about it when loading documents.  (WebDataSource is the API wrapper for
> DocumentLoader.)

We decided to investigate a way to use shared memory to share the list with all of the renderer processes.

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> Thanks for the quick response.
> 
> (In reply to comment #3)
> > (From update of attachment 123699 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=123699&action=review
> > 
> > > Source/WebKit/chromium/public/platform/WebURLResponse.h:177
> > > +    WEBKIT_EXPORT bool needsEncodingDetection() const;
> > 
> > This feels like a layering violation.  Why does the network stack need to know
> > about this?
> 
> We needed to lookup the list of sites to turn on the encoding detector very fast.
> And we wanted to avoid having to sync the list to all of the renderer processes.

I see.


> > Maybe this should be a method on WebDataSource instead?  It is not
> > clear to me where you call setNeedsEncodingDetection, but it looks like you only
> > care about it when loading documents.  (WebDataSource is the API wrapper for
> > DocumentLoader.)
> 
> We decided to investigate a way to use shared memory to share the list with all of the renderer processes.

There are other options.  If you only care about modifying this for top-level navigations, then you could do like we do for per-origin zoom settings and content settings.  See AsyncResourceHandler::OnResponseStarted.

[Keishi Hattori]

(In reply to comment #5)
> > > Maybe this should be a method on WebDataSource instead?  It is not
> > > clear to me where you call setNeedsEncodingDetection, but it looks like you only
> > > care about it when loading documents.  (WebDataSource is the API wrapper for
> > > DocumentLoader.)
> > 
> > We decided to investigate a way to use shared memory to share the list with all of the renderer processes.
> 
> There are other options.  If you only care about modifying this for top-level navigations, then you could do like we do for per-origin zoom settings and content settings.  See AsyncResourceHandler::OnResponseStarted.

Thanks for the pointer. That seems like a good idea.
We will try to implement that and send our design doc to chromium-dev@ once its working.

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> > There are other options.  If you only care about modifying this for top-level navigations, then you could do like we do for per-origin zoom settings and content settings.  See AsyncResourceHandler::OnResponseStarted.
> 
> Thanks for the pointer. That seems like a good idea.
> We will try to implement that and send our design doc to chromium-dev@ once its working.

By the way, I misspoke about content settings.  Those are no longer handled like zoom settings.  Instead, the renderer has a complete copy of the content settings.