Issue 2178213002: Add note page to settings.

jdufault

Description was changed from ========== wip BUG= ========== to ========== wip BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========

4 years, 5 months ago (2016-07-26 00:24:53 UTC) #1

jdufault

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

4 years, 5 months ago (2016-07-26 00:24:59 UTC) #2

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-26 00:25:24 UTC) #3

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-26 00:36:34 UTC) #4

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-generic_chromium_compile_only_ng/builds/173060)

4 years, 5 months ago (2016-07-26 00:36:35 UTC) #5

jdufault

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

4 years, 4 months ago (2016-07-26 17:48:29 UTC) #6

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 17:48:46 UTC) #7

jdufault

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

4 years, 4 months ago (2016-07-26 18:50:32 UTC) #8

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 18:51:02 UTC) #10

jdufault

Description was changed from ========== wip BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add note page to ...

4 years, 4 months ago (2016-07-26 18:51:55 UTC) #12

jdufault

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

4 years, 4 months ago (2016-07-26 18:55:02 UTC) #13

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 18:55:36 UTC) #15

jdufault

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui/settings/chromeos/note_handler.cc File chrome/browser/ui/webui/settings/chromeos/note_handler.cc (right): https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui/settings/chromeos/note_handler.cc#newcode23 chrome/browser/ui/webui/settings/chromeos/note_handler.cc:23: NoteHandler::NoteHandler(content::WebUIDataSource* html_source) { For right now, the handler is ...

4 years, 4 months ago (2016-07-26 19:42:56 UTC) #16

jdufault

jdufault@chromium.org changed reviewers: + stevenjb@chromium.org

4 years, 4 months ago (2016-07-26 19:43:10 UTC) #17

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 19:58:19 UTC) #19

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-07-26 19:58:20 UTC) #20

stevenjb

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/device_page.html File chrome/browser/resources/settings/device_page/device_page.html (right): https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/device_page.html#newcode38 chrome/browser/resources/settings/device_page/device_page.html:38: <div id="noteRow" class="settings-box" on-tap="onNoteTap_" id not needed? https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/device_page.html#newcode42 chrome/browser/resources/settings/device_page/device_page.html:42: ...

4 years, 4 months ago (2016-07-26 23:21:39 UTC) #21

jdufault

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/device_page.html File chrome/browser/resources/settings/device_page/device_page.html (right): https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/device_page.html#newcode38 chrome/browser/resources/settings/device_page/device_page.html:38: <div id="noteRow" class="settings-box" on-tap="onNoteTap_" On 2016/07/26 23:21:39, stevenjb wrote: ...

4 years, 4 months ago (2016-07-27 20:55:02 UTC) #22

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:38: <div
id="noteRow" class="settings-box" on-tap="onNoteTap_"
On 2016/07/26 23:21:39, stevenjb wrote:
> id not needed?

Done.

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:42: <div
class="middle">Note</div>
On 2016/07/26 23:21:39, stevenjb wrote:
> We should do i18n now, it's easy to miss something later even with a comment.
> Nothing will get translated until the next release.
> 

Done.

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:65: <template
is="dom-if" name="note">
On 2016/07/26 23:21:39, stevenjb wrote:
> if=?

Done. I've added it in another <template> wrapper, since adding it to the
existing one didn't work.

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/note.html (right):

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/note.html:11: }
On 2016/07/26 23:21:39, stevenjb wrote:
> Not needed, see below.

Done.

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/note.html:26: target="_blank">Find
note apps</a>
On 2016/07/26 23:21:39, stevenjb wrote:
> Add is="action-link" which will pick up styling, see other examples in this
> directory.

Done.

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/note.html:30: <div
class="settings-box"></div>
On 2016/07/26 23:21:39, stevenjb wrote:
> This shouldn't be needed...

I'm fine having it not be there, but the mocks include it. I've dropped it for
the time being.

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/note_handler.cc (right):

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/note_handler.cc:23:
NoteHandler::NoteHandler(content::WebUIDataSource* html_source) {
On 2016/07/26 23:21:39, stevenjb wrote:
> On 2016/07/26 19:42:56, jdufault wrote:
> > For right now, the handler is very simple, so it is reasonable to remove it
> > entirely and inline this code into md_settings_ui.cc.
> > 
> > However, in the future there will be additional settings in this page to
> select
> > the default note-taking application. This will require adding a message
> handler
> > here, so I figured I'd implement the boilerplate now rather than later to
> reduce
> > unnecessary churn.
> 
> Ideally we will use an extension API instead of a message handler, let's omit
> this until / unless we need it.

Most settings UI appears to use an message handler - is there any reason why
this necessitates an extension API? The extension API would have one method,
getAppIdsSupportingAction(DOMString actionId), or something like that.

stevenjb

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/note.html File chrome/browser/resources/settings/device_page/note.html (right): https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/note.html#newcode30 chrome/browser/resources/settings/device_page/note.html:30: <div class="settings-box"></div> On 2016/07/27 20:55:01, jdufault wrote: > On ...

4 years, 4 months ago (2016-07-27 22:29:42 UTC) #23

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/note.html (right):

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/note.html:30: <div
class="settings-box"></div>
On 2016/07/27 20:55:01, jdufault wrote:
> On 2016/07/26 23:21:39, stevenjb wrote:
> > This shouldn't be needed...
> 
> I'm fine having it not be there, but the mocks include it. I've dropped it for
> the time being.

I added a comment in folio. That is inconsistent with other settings mocks. If
we change the design we should add a better way, e.g. class="settings-box last"

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/note_handler.cc (right):

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/note_handler.cc:23:
NoteHandler::NoteHandler(content::WebUIDataSource* html_source) {
On 2016/07/27 20:55:02, jdufault wrote:
> On 2016/07/26 23:21:39, stevenjb wrote:
> > On 2016/07/26 19:42:56, jdufault wrote:
> > > For right now, the handler is very simple, so it is reasonable to remove
it
> > > entirely and inline this code into md_settings_ui.cc.
> > > 
> > > However, in the future there will be additional settings in this page to
> > select
> > > the default note-taking application. This will require adding a message
> > handler
> > > here, so I figured I'd implement the boilerplate now rather than later to
> > reduce
> > > unnecessary churn.
> > 
> > Ideally we will use an extension API instead of a message handler, let's
omit
> > this until / unless we need it.
> 
> Most settings UI appears to use an message handler - is there any reason why
> this necessitates an extension API? The extension API would have one method,
> getAppIdsSupportingAction(DOMString actionId), or something like that.

Some day, we would like to be able to run device specific settings from the
login screen or from a kiosk app. Ideally Settings would be run as a stand-alone
app with no associated Chrome instance, i.e. AppShell. In that environment we
would need to rely on extension APIs that exist in AppShell.

We're a ways from that being the case, so I wouldn't discard this code, but I
would still prefer to only add it when we actually need it. Priorities have a
habit of changing.

https://codereview.chromium.org/2178213002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2178213002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:65: <template
is="dom-if" name="note">
I'm confused. What is this template supposed to be doing?

jdufault

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/note.html File chrome/browser/resources/settings/device_page/note.html (right): https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resources/settings/device_page/note.html#newcode30 chrome/browser/resources/settings/device_page/note.html:30: <div class="settings-box"></div> On 2016/07/27 22:29:42, stevenjb wrote: > On ...

4 years, 4 months ago (2016-07-28 17:54:40 UTC) #25

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/note.html (right):

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/note.html:30: <div
class="settings-box"></div>
On 2016/07/27 22:29:42, stevenjb wrote:
> On 2016/07/27 20:55:01, jdufault wrote:
> > On 2016/07/26 23:21:39, stevenjb wrote:
> > > This shouldn't be needed...
> > 
> > I'm fine having it not be there, but the mocks include it. I've dropped it
for
> > the time being.
> 
> I added a comment in folio. That is inconsistent with other settings mocks. If
> we change the design we should add a better way, e.g. class="settings-box
last"

Sounds good, thanks!

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/note_handler.cc (right):

https://codereview.chromium.org/2178213002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/note_handler.cc:23:
NoteHandler::NoteHandler(content::WebUIDataSource* html_source) {
On 2016/07/27 22:29:42, stevenjb wrote:
> On 2016/07/27 20:55:02, jdufault wrote:
> > On 2016/07/26 23:21:39, stevenjb wrote:
> > > On 2016/07/26 19:42:56, jdufault wrote:
> > > > For right now, the handler is very simple, so it is reasonable to remove
> it
> > > > entirely and inline this code into md_settings_ui.cc.
> > > > 
> > > > However, in the future there will be additional settings in this page to
> > > select
> > > > the default note-taking application. This will require adding a message
> > > handler
> > > > here, so I figured I'd implement the boilerplate now rather than later
to
> > > reduce
> > > > unnecessary churn.
> > > 
> > > Ideally we will use an extension API instead of a message handler, let's
> omit
> > > this until / unless we need it.
> > 
> > Most settings UI appears to use an message handler - is there any reason why
> > this necessitates an extension API? The extension API would have one method,
> > getAppIdsSupportingAction(DOMString actionId), or something like that.
> 
> Some day, we would like to be able to run device specific settings from the
> login screen or from a kiosk app. Ideally Settings would be run as a
stand-alone
> app with no associated Chrome instance, i.e. AppShell. In that environment we
> would need to rely on extension APIs that exist in AppShell.
> 
> We're a ways from that being the case, so I wouldn't discard this code, but I
> would still prefer to only add it when we actually need it. Priorities have a
> habit of changing.

Done.

https://codereview.chromium.org/2178213002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2178213002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:65: <template
is="dom-if" name="note">
On 2016/07/27 22:29:42, stevenjb wrote:
> I'm confused. What is this template supposed to be doing?

It's my understanding this activates the element if the route is on note.

stevenjb

lgtm https://codereview.chromium.org/2178213002/diff/80001/chrome/browser/resources/settings/device_page/device_page.html File chrome/browser/resources/settings/device_page/device_page.html (right): https://codereview.chromium.org/2178213002/diff/80001/chrome/browser/resources/settings/device_page/device_page.html#newcode65 chrome/browser/resources/settings/device_page/device_page.html:65: <template is="dom-if" name="note"> On 2016/07/28 17:54:40, jdufault wrote: ...

4 years, 4 months ago (2016-07-28 18:01:47 UTC) #26

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-28 19:11:26 UTC) #28

commit-bot: I haz the power

Description was changed from ========== Add note page to settings. BUG=616112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== ...

4 years, 4 months ago (2016-07-28 20:22:24 UTC) #29

commit-bot: I haz the power

Description was changed from ========== Add note page to settings. BUG=616112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== ...

4 years, 4 months ago (2016-07-28 20:23:59 UTC) #31

commit-bot: I haz the power

4 years, 4 months ago (2016-07-28 20:24:00 UTC) #32

Message was sent while issue was closed.

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

Issue 2178213002: Add note page to settings. (Closed)

Description

Patch Set 1 : Initial upload #

Patch Set 2 : Comments #

Patch Set 3 : Address comments and add utility method #

Messages