Gzip compress chrome://accessibility resources

Gzip compress chrome://accessibility resources

BUG=619091
R=agrieve@chromium.org

Committed: https://crrev.com/6a700b0e8bddef30af43a7ceaff4ad8dd4693a1b
Cr-Commit-Position: refs/heads/master@{#425197}

[Dan Beam, 2016-10-05 17:40:42]

i haven't actually tried this yet, but it'll probbbbably work

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_ui.cc:145: static
AccessibilityDataSource* Create(const std::string& source_name) {
i had to do this because a super is ref-counted

[chromium-style] Classes that are ref-counted should have destructors that are
declared protected or private.

[Dan Beam, 2016-10-05 17:40:49]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 17:41:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 18:31:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 18:31:08]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-10-05 18:40:20]

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_ui.cc:145: static
AccessibilityDataSource* Create(const std::string& source_name) {
On 2016/10/05 17:40:42, Dan Beam wrote:
> i had to do this because a super is ref-counted
> 
> [chromium-style] Classes that are ref-counted should have destructors that are
> declared protected or private.

A bit annoying that you need to subclass for this purpose. What do you think of
having the DisableI18nAndUseGzipForAllPaths() just apply to all paths witihin
path_to_idr_map_ or default_resource_? It'd be very strange to have a gzipped
resource not backed by a .pak file.

[Dan Beam, 2016-10-05 19:03:36]

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_ui.cc:145: static
AccessibilityDataSource* Create(const std::string& source_name) {
On 2016/10/05 18:40:19, agrieve wrote:
> On 2016/10/05 17:40:42, Dan Beam wrote:
> > i had to do this because a super is ref-counted
> > 
> > [chromium-style] Classes that are ref-counted should have destructors that
are
> > declared protected or private.
> 
> A bit annoying that you need to subclass for this purpose. What do you think
of
> having the DisableI18nAndUseGzipForAllPaths() just apply to all paths witihin
> path_to_idr_map_ or default_resource_? It'd be very strange to have a gzipped
> resource not backed by a .pak file.

yeah, that might work.  i could also just make a way to tell the data source
which paths to ignore (i.e. a std::vector<std::string> excluded_paths_; in a
super somewhere)

[agrieve, 2016-10-05 19:09:33]

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_ui.cc:145: static
AccessibilityDataSource* Create(const std::string& source_name) {
On 2016/10/05 19:03:36, Dan Beam wrote:
> On 2016/10/05 18:40:19, agrieve wrote:
> > On 2016/10/05 17:40:42, Dan Beam wrote:
> > > i had to do this because a super is ref-counted
> > > 
> > > [chromium-style] Classes that are ref-counted should have destructors that
> are
> > > declared protected or private.
> > 
> > A bit annoying that you need to subclass for this purpose. What do you think
> of
> > having the DisableI18nAndUseGzipForAllPaths() just apply to all paths
witihin
> > path_to_idr_map_ or default_resource_? It'd be very strange to have a
gzipped
> > resource not backed by a .pak file.
> 
> yeah, that might work.  i could also just make a way to tell the data source
> which paths to ignore (i.e. a std::vector<std::string> excluded_paths_; in a
> super somewhere)

I'd be supportive of that once we need it (but we don't yet).

There are also some internals pages that go through translation where it might
make sense to compress just the .js and .css. Even for these though, we could
just have a separate bool for the default resource vs things in
path_to_idr_map_.

[Dan Beam, 2016-10-12 03:33:15]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 03:33:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2016-10-12 04:08:57]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-10-12 04:09:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2016-10-12 04:32:30]

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/2399553002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_ui.cc:145: static
AccessibilityDataSource* Create(const std::string& source_name) {
On 2016/10/05 19:09:33, agrieve wrote:
> On 2016/10/05 19:03:36, Dan Beam wrote:
> > On 2016/10/05 18:40:19, agrieve wrote:
> > > On 2016/10/05 17:40:42, Dan Beam wrote:
> > > > i had to do this because a super is ref-counted
> > > > 
> > > > [chromium-style] Classes that are ref-counted should have destructors
that
> > are
> > > > declared protected or private.
> > > 
> > > A bit annoying that you need to subclass for this purpose. What do you
think
> > of
> > > having the DisableI18nAndUseGzipForAllPaths() just apply to all paths
> witihin
> > > path_to_idr_map_ or default_resource_? It'd be very strange to have a
> gzipped
> > > resource not backed by a .pak file.
> > 
> > yeah, that might work.  i could also just make a way to tell the data source
> > which paths to ignore (i.e. a std::vector<std::string> excluded_paths_; in a
> > super somewhere)
> 
> I'd be supportive of that once we need it (but we don't yet).
> 
> There are also some internals pages that go through translation where it might
> make sense to compress just the .js and .css. Even for these though, we could
> just have a separate bool for the default resource vs things in
> path_to_idr_map_.

so this won't work (I tried here[1]), because

GET /
GET /non-existent
GET /strings.js
GET /dynamic-thing.json

all return the default resource.  you can special case the json_path_ (i.e.
"strings.js"), but otherwise it's not possible to disambiguate.

if we disable the default resource, things blow up for /non-existent (and we'd
prefer them to just work if typing chrome://accessibility/blah) and special code
would be needed for "" or "/".

so I added an excluded_paths_. check it out.

[1] https://codereview.chromium.org/2399553002/#ps40001

[Dan Beam, 2016-10-12 04:37:14]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 04:37:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 05:57:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 05:57:53]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-10-12 14:19:36]

Added what I think would have been a bit simpler, but I don't feel strongly
(more important to just land something), so lgtm.

https://codereview.chromium.org/2399553002/diff/40001/content/browser/webui/w...
File content/browser/webui/web_ui_data_source_impl.cc (right):

https://codereview.chromium.org/2399553002/diff/40001/content/browser/webui/w...
content/browser/webui/web_ui_data_source_impl.cc:78:
parent_->GetResourceIdFromPath(path) != -1;
I think this would have worked:

if (path == json_path_)
  return false;

bool is_default_path = parent_->GetResourceIdFromPath(path) ==
default_resource_;
return use_gzip_for_default_resource_ && is_default_path ||
           use_gzip_for_subresources_ && !is_default_path;

[Dan Beam, 2016-10-12 16:26:32]

https://codereview.chromium.org/2399553002/diff/40001/content/browser/webui/w...
File content/browser/webui/web_ui_data_source_impl.cc (right):

https://codereview.chromium.org/2399553002/diff/40001/content/browser/webui/w...
content/browser/webui/web_ui_data_source_impl.cc:78:
parent_->GetResourceIdFromPath(path) != -1;
On 2016/10/12 14:19:36, agrieve wrote:
> I think this would have worked:
> 
> if (path == json_path_)

this breaks when no json_path_ is set, but a .empty() check would probably fix

>   return false;
> 
> bool is_default_path = parent_->GetResourceIdFromPath(path) ==
> default_resource_;
> return use_gzip_for_default_resource_ && is_default_path ||
>            use_gzip_for_subresources_ && !is_default_path;

kDataPath would match the is_default_path and would be gzipped (even though the
contents is not), and there'd be a encoding failure / broken resource.

[agrieve, 2016-10-12 16:31:23]

On 2016/10/12 16:26:32, Dan Beam wrote:
>
https://codereview.chromium.org/2399553002/diff/40001/content/browser/webui/w...
> File content/browser/webui/web_ui_data_source_impl.cc (right):
> 
>
https://codereview.chromium.org/2399553002/diff/40001/content/browser/webui/w...
> content/browser/webui/web_ui_data_source_impl.cc:78:
> parent_->GetResourceIdFromPath(path) != -1;
> On 2016/10/12 14:19:36, agrieve wrote:
> > I think this would have worked:
> > 
> > if (path == json_path_)
> 
> this breaks when no json_path_ is set, but a .empty() check would probably fix
> 
> >   return false;
> > 
> > bool is_default_path = parent_->GetResourceIdFromPath(path) ==
> > default_resource_;
> > return use_gzip_for_default_resource_ && is_default_path ||
> >            use_gzip_for_subresources_ && !is_default_path;
> 
> kDataPath would match the is_default_path and would be gzipped (even though
the
> contents is not), and there'd be a encoding failure / broken resource.

Ah, didn't catch that :(. Thanks for spelling it out! lgtm.

[Dan Beam, 2016-10-13 05:11:04]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-10-13 05:11:15]

The CQ bit was unchecked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-10-13 05:11:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2016-10-13 05:11:49]

dbeam@chromium.org changed reviewers:
+ creis@chromium.org

[Dan Beam, 2016-10-13 05:11:50]

+creis@ for content/OWNERS

[Charlie Reis, 2016-10-13 17:04:44]

(I'm away today, but here's a quick comment.  Happy to follow up tomorrow or
when I get a moment.)

https://codereview.chromium.org/2399553002/diff/80001/content/public/browser/...
File content/public/browser/web_ui_data_source.h (right):

https://codereview.chromium.org/2399553002/diff/80001/content/public/browser/...
content/public/browser/web_ui_data_source.h:100: virtual void
ExcludePathFromGzip(const std::string& path) = 0;
This is in content/public but doesn't appear to be used outside content/.  Can
we just have it in the impl file until there's a use outside content?

(Also, please add a comment if we leave it here.)

http://www.chromium.org/developers/content-module/content-api
"only expose methods in the public API that embedders need. If a method is only
used by other code in content, it belongs in foo_impl.h and not foo.h"

[Dan Beam, 2016-10-13 17:53:46]

https://codereview.chromium.org/2399553002/diff/80001/content/public/browser/...
File content/public/browser/web_ui_data_source.h (right):

https://codereview.chromium.org/2399553002/diff/80001/content/public/browser/...
content/public/browser/web_ui_data_source.h:100: virtual void
ExcludePathFromGzip(const std::string& path) = 0;
On 2016/10/13 17:04:43, Charlie Reis (Away till 10-14) wrote:
> This is in content/public but doesn't appear to be used outside content/.  Can
> we just have it in the impl file until there's a use outside content?
> 
> (Also, please add a comment if we leave it here.)
> 
> http://www.chromium.org/developers/content-module/content-api
> "only expose methods in the public API that embedders need. If a method is
only
> used by other code in content, it belongs in foo_impl.h and not foo.h"

yep, sorry about that, most of the webui pages i mess with are in chrome/.  one
of them will probably eventually find the need for this, but until then: Done.

[Charlie Reis, 2016-10-13 19:40:12]

Thanks!  RS LGTM (mostly deferring to agrieve on the implementation).

https://codereview.chromium.org/2399553002/diff/100001/content/browser/webui/...
File content/browser/webui/web_ui_data_source_impl.h (right):

https://codereview.chromium.org/2399553002/diff/100001/content/browser/webui/...
content/browser/webui/web_ui_data_source_impl.h:53: void
ExcludePathFromGzip(const std::string& path);
nit: Please add a comment, since it's not obvious what effect this has at first.

[Dan Beam, 2016-10-13 21:49:47]

https://codereview.chromium.org/2399553002/diff/100001/content/browser/webui/...
File content/browser/webui/web_ui_data_source_impl.h (right):

https://codereview.chromium.org/2399553002/diff/100001/content/browser/webui/...
content/browser/webui/web_ui_data_source_impl.h:53: void
ExcludePathFromGzip(const std::string& path);
On 2016/10/13 19:40:12, Charlie Reis (Away till 10-14) wrote:
> nit: Please add a comment, since it's not obvious what effect this has at
first.

Done.

[Dan Beam, 2016-10-13 21:49:59]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-10-13 21:49:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2399553002/#ps120001
(title: "asdf")

[commit-bot: I haz the power, 2016-10-13 21:50:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 22:45:49]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-10-13 22:47:12]

Description was changed from

==========
Gzip compress chrome://accessibility resources

BUG=619091
R=agrieve@chromium.org
==========

to

==========
Gzip compress chrome://accessibility resources

BUG=619091
R=agrieve@chromium.org

Committed: https://crrev.com/6a700b0e8bddef30af43a7ceaff4ad8dd4693a1b
Cr-Commit-Position: refs/heads/master@{#425197}
==========

[commit-bot: I haz the power, 2016-10-13 22:47:13]

Patchset 6 (id:??) landed as
https://crrev.com/6a700b0e8bddef30af43a7ceaff4ad8dd4693a1b
Cr-Commit-Position: refs/heads/master@{#425197}