[MD Bookmarks] Add skeleton for Material Design Bookmarks

[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b560c25322e96b12e72689fbfa7c1cc977e4661c
Cr-Commit-Position: refs/heads/master@{#428961}

[calamity, 2016-10-25 02:10:12]

Description was changed from

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
==========

to

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-10-25 03:36:30]

Patchset #1 (id:1) has been deleted

[calamity, 2016-10-25 03:45:59]

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

[calamity, 2016-10-25 03:46:00]

I think I need to talk to chrome-eng-review@ to add another component extension.

Also, i18n is going to be.. interesting.

[calamity, 2016-10-25 07:26:59]

Patchset #1 (id:20001) has been deleted

[Dan Beam, 2016-10-25 18:05:31]

Description was changed from

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-10-25 18:05:52]

looks generally good to me

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:465: int bookmark_manifest_id =
IsMaterialDesignBookmarksEnabled()
can we cache this result?

also, can we just do the feature enabledness check in this method only?

why do we need another method in an anonymous namespace?

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/manifest.json (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/manifest.json:4: "version": "0.2",
some day we might even make it to 1!

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:113: "PushMessagingBackgroundMode",
base::FEATURE_DISABLED_BY_DEFAULT};
Y U MOVE?

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/extension...
File chrome/common/extensions/extension_constants.h (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/extension...
chrome/common/extensions/extension_constants.h:38: extern const char
kMDBookmarkManagerId[];
arguuuuuuuuably kMdBookmarkManagerId
                 ^^
as in MD -> Md

https://codereview.chromium.org/2450623002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2450623002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:102929: +             Manager)"/>
can you put "Material Design" in this somewhere?

[calamity, 2016-10-28 05:55:16]

Patchset #3 (id:80001) has been deleted

[calamity, 2016-10-28 05:58:22]

Switched this out to use native WebUI. \o/

I checked that overriding with an extension still works and it seems to, so
that's nice.

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:465: int bookmark_manifest_id =
IsMaterialDesignBookmarksEnabled()
On 2016/10/25 18:05:51, Dan Beam wrote:
> can we cache this result?
> 
> also, can we just do the feature enabledness check in this method only?
> 
> why do we need another method in an anonymous namespace?

Done.

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/manifest.json (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/manifest.json:4: "version": "0.2",
On 2016/10/25 18:05:51, Dan Beam wrote:
> some day we might even make it to 1!

Acknowledged.

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:113: "PushMessagingBackgroundMode",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/10/25 18:05:51, Dan Beam wrote:
> Y U MOVE?

Match order in header. MD WebUIs should stick together.

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/extension...
File chrome/common/extensions/extension_constants.h (right):

https://codereview.chromium.org/2450623002/diff/40001/chrome/common/extension...
chrome/common/extensions/extension_constants.h:38: extern const char
kMDBookmarkManagerId[];
On 2016/10/25 18:05:51, Dan Beam wrote:
> arguuuuuuuuably kMdBookmarkManagerId
>                  ^^
> as in MD -> Md

git grep approves.

https://codereview.chromium.org/2450623002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2450623002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:102929: +             Manager)"/>
On 2016/10/25 18:05:51, Dan Beam wrote:
> can you put "Material Design" in this somewhere?

Undone. We'll just be using chrome://bookmarks now (which somehow has some logs
to it already).

[Dan Beam, 2016-10-29 01:10:00]

why are we adding all the strings now? :/

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:924: "webui/md_bookmarks_ui.h",
nit: should we make an md_bookmarks/?  it's been generally nicer, over the
years, to work with dedicated sub-dirs (for OWNERS and other stuff)

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:801: return
MdBookmarksUI::GetFaviconResourceBytes(scale_factor);
why do we want these favicons to ever be different?

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:802: }
nit: no curlies on 1-liners

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/md_bookmarks_ui.cc (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_bookmarks_ui.cc:71:
source->AddLocalizedString("cancel", IDS_CANCEL);
dang, can we add this as needed?

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_bookmarks_ui.cc:94: base::RefCountedMemory*
MdBookmarksUI::GetFaviconResourceBytes(
can we de-dupe this with other bookmarks?

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/_api_features.json:122: "chrome://bookmarks/*"
are we planning on using this extension API for the new page?

[Dan Beam, 2016-10-29 01:10:27]

(other than that, this is pretty much ready to go, IMO)

i'm glad this sidesteps the "should we use Polymer 2" discussions that are
cropping up

[calamity, 2016-11-01 02:32:54]

Patchset #4 (id:120001) has been deleted

[calamity, 2016-11-01 02:37:58]

calamity@chromium.org changed reviewers:
+ benwells@chromium.org, isherman@chromium.org

[calamity, 2016-11-01 02:37:59]

+benwells for component_loader.cc and _api_features.json
+isherman for histograms.xml

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:801: return
MdBookmarksUI::GetFaviconResourceBytes(scale_factor);
On 2016/10/29 01:10:00, Dan Beam wrote:
> why do we want these favicons to ever be different?

Removed.

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:802: }
On 2016/10/29 01:10:00, Dan Beam wrote:
> nit: no curlies on 1-liners

N/A.

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/md_bookmarks_ui.cc (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_bookmarks_ui.cc:71:
source->AddLocalizedString("cancel", IDS_CANCEL);
On 2016/10/29 01:10:00, Dan Beam wrote:
> dang, can we add this as needed?

Good point. Done.

https://codereview.chromium.org/2450623002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_bookmarks_ui.cc:94: base::RefCountedMemory*
MdBookmarksUI::GetFaviconResourceBytes(
On 2016/10/29 01:10:00, Dan Beam wrote:
> can we de-dupe this with other bookmarks?

Done.

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/_api_features.json:122: "chrome://bookmarks/*"
On 2016/10/29 01:10:00, Dan Beam wrote:
> are we planning on using this extension API for the new page?

I think we should. It's what the existing bookmark manager uses and it'd be
annoying to reimplement as a WebUI message handler. Any reason not to?

[Ilya Sherman, 2016-11-01 02:39:40]

histograms.xml lgtm

[benwells, 2016-11-01 03:46:36]

those two files lgtm

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/component_loader.cc:26: #include
"chrome/common/chrome_features.h"
Nit: I don't think this include is needed

[Dan Beam, 2016-11-01 06:34:16]

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/_api_features.json:122: "chrome://bookmarks/*"
On 2016/11/01 02:37:59, calamity wrote:
> On 2016/10/29 01:10:00, Dan Beam wrote:
> > are we planning on using this extension API for the new page?
> 
> I think we should. It's what the existing bookmark manager uses and it'd be
> annoying to reimplement as a WebUI message handler. Any reason not to?

i haven't found extension APIs all too helpful on settings

[Dan Beam, 2016-11-01 06:34:17]

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/2450623002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/_api_features.json:122: "chrome://bookmarks/*"
On 2016/11/01 02:37:59, calamity wrote:
> On 2016/10/29 01:10:00, Dan Beam wrote:
> > are we planning on using this extension API for the new page?
> 
> I think we should. It's what the existing bookmark manager uses and it'd be
> annoying to reimplement as a WebUI message handler. Any reason not to?

i haven't found extension APIs all too helpful on settings

[Dan Beam, 2016-11-01 06:38:28]

lgtm

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/browser...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/browser...
chrome/browser/browser_resources.grd:247: <include
name="IDR_MD_BOOKMARKS_BOOKMARKS_HTML"
file="resources\md_bookmarks\bookmarks.html" flattenhtml="true" type="BINDATA"
/>
don't add flattenhtml="true" until necessary

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc (right):

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:18: #include
"ui/base/resource/resource_bundle.h"
i don't think these are all needed

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.h (right):

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.h:8: #include
"base/gtest_prod_util.h"
nit: not needed

https://codereview.chromium.org/2450623002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.h:11: #include
"ui/base/layout.h"
nit: not needed

[calamity, 2016-11-01 07:27:05]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-11-01 07:27:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, isherman@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2450623002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-01 07:27:19]

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-11-01 08:30:39]

Description was changed from

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-01 08:30:41]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-11-01 08:32:53]

Description was changed from

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add skeleton for Material Design Bookmarks

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b560c25322e96b12e72689fbfa7c1cc977e4661c
Cr-Commit-Position: refs/heads/master@{#428961}
==========

[commit-bot: I haz the power, 2016-11-01 08:32:55]

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