Elide origins displayed on web notifications.

Elide origins displayed on web notifications.

WebPush displays notifications including the origin in the context message
field. When such origin is too long it is now elided in a secure acceptable
way.

BUG=509834
Committed: https://crrev.com/53a6dde573f020d57b4fff5923f272f98da66e27
Cr-Commit-Position: refs/heads/master@{#344455}

[Miguel Garcia, 2015-08-14 15:50:43]

miguelg@chromium.org changed reviewers:
+ mukai@chromium.org

[Miguel Garcia, 2015-08-14 15:50:43]

Thanks in advance for the review!

Note that I have checked with ui/OWNERS to make sure they were ok with adding
the new DEPS rule.

[Jun Mukai, 2015-08-14 17:17:07]

Also, you may want to modify
chrome/browser/notifications/notification_ui_manager_android.cc for the handling
of origins.

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/DEPS
File ui/message_center/DEPS (right):

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/DEPS#newc...
ui/message_center/DEPS:2: "+components/url_formatter/elide_url.h",
I think it's okay to add the entire "components/url_formatter".

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/notificat...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/notificat...
ui/message_center/notification.h:36: struct MESSAGE_CENTER_EXPORT ContextMessage
{
I prefer to keep string16 for context message. Could you:
- move the 'origin_url' field from chrome/browser/notifications/notification.h
to ui/message_center/notification.h
- use this origin_url if context_message is empty (and origin_url isn't
extension origin), apply the url_formatter to the origin_url

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/views/con...
File ui/message_center/views/constants.h (left):

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/views/con...
ui/message_center/views/constants.h:24: const int kTextLeftPadding = kIconSize +
message_center::kIconToTextPadding;
Please do not move views specific constants to generic message_center_style.h

[Miguel Garcia, 2015-08-14 18:28:45]

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/notificat...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/notificat...
ui/message_center/notification.h:36: struct MESSAGE_CENTER_EXPORT ContextMessage
{
On 2015/08/14 17:17:07, Jun Mukai wrote:
> I prefer to keep string16 for context message. Could you:
> - move the 'origin_url' field from chrome/browser/notifications/notification.h
> to ui/message_center/notification.h
> - use this origin_url if context_message is empty (and origin_url isn't
> extension origin), apply the url_formatter to the origin_url

Will give a shot, now that you mention it I recall Peter saying exactly the same
thing about not switching to std::string but I cannot remeber the reasoning,
could you elaborate why not make the change?

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/views/con...
File ui/message_center/views/constants.h (left):

https://codereview.chromium.org/1292003004/diff/1/ui/message_center/views/con...
ui/message_center/views/constants.h:24: const int kTextLeftPadding = kIconSize +
message_center::kIconToTextPadding;
On 2015/08/14 17:17:07, Jun Mukai wrote:
> Please do not move views specific constants to generic message_center_style.h

Ok, any advice on where to put them so they can be reshared in mac and all other
platforms?

[Miguel Garcia, 2015-08-17 17:27:55]

Jun, 

  I thought about your suggestion and implemented something similar. I still
think that the controller (in notification.h) should tell the view (in
notification_view.h) what to paint exactly so I kept the boolean to tell it it
should ignore the explicit content message and just format the origin.

  I then switch the content message string back to string16 and moved origin_url
to message_center notification as suggested. The latter means changing the
Notification constructor which as you might imagine involves changing dozens of
classes, especially in ChromeOs

I have been uploading the same cl in
https://chromiumcodereview.appspot.com/1235253009/ (so that I can switch & patch
between mac and Linux) and before going any further I would prefer if you have a
look there just to make sure this is going in the right direction since the
proposed patch is going to be huge otherwise.



On 2015/08/14 18:28:45, Miguel Garcia wrote:
>
https://codereview.chromium.org/1292003004/diff/1/ui/message_center/notificat...
> File ui/message_center/notification.h (right):
> 
>
https://codereview.chromium.org/1292003004/diff/1/ui/message_center/notificat...
> ui/message_center/notification.h:36: struct MESSAGE_CENTER_EXPORT
ContextMessage
> {
> On 2015/08/14 17:17:07, Jun Mukai wrote:
> > I prefer to keep string16 for context message. Could you:
> > - move the 'origin_url' field from
chrome/browser/notifications/notification.h
> > to ui/message_center/notification.h
> > - use this origin_url if context_message is empty (and origin_url isn't
> > extension origin), apply the url_formatter to the origin_url
> 
> Will give a shot, now that you mention it I recall Peter saying exactly the
same
> thing about not switching to std::string but I cannot remeber the reasoning,
> could you elaborate why not make the change?
> 
>
https://codereview.chromium.org/1292003004/diff/1/ui/message_center/views/con...
> File ui/message_center/views/constants.h (left):
> 
>
https://codereview.chromium.org/1292003004/diff/1/ui/message_center/views/con...
> ui/message_center/views/constants.h:24: const int kTextLeftPadding = kIconSize
+
> message_center::kIconToTextPadding;
> On 2015/08/14 17:17:07, Jun Mukai wrote:
> > Please do not move views specific constants to generic
message_center_style.h
> 
> Ok, any advice on where to put them so they can be reshared in mac and all
other
> platforms?

[Jun Mukai, 2015-08-17 18:36:15]

Also please consider the Ryan's comment in
https://chromiumcodereview.appspot.com/1235253009/.

You may want to move url_formatter into ui (under ui/gfx for example), or the
url_formatting should be done in somewhere else.

Another design suggestion -- do nothing for message_center side, but specify the
elided URL as the context message in platform_notification_service_impl.cc?

That way, the dependency confusion doesn't happen, and the change would be
slightly simpler.

https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
File ui/message_center/notification.h (right):

https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
ui/message_center/notification.h:36: struct MESSAGE_CENTER_EXPORT ContextMessage
{
On 2015/08/14 18:28:45, Miguel Garcia wrote:
> On 2015/08/14 17:17:07, Jun Mukai wrote:
> > I prefer to keep string16 for context message. Could you:
> > - move the 'origin_url' field from
chrome/browser/notifications/notification.h
> > to ui/message_center/notification.h
> > - use this origin_url if context_message is empty (and origin_url isn't
> > extension origin), apply the url_formatter to the origin_url
> 
> Will give a shot, now that you mention it I recall Peter saying exactly the
same
> thing about not switching to std::string but I cannot remeber the reasoning,
> could you elaborate why not make the change?

string16 is the text representation on UI side, so it's a good design to convert
into string16 at the earliest point and keep it as far as possible -- also it
will reduce the number of conversions / copies.

I think, however, in your new design, this might not satisfy because you want to
the message into a GURL anyways.
That's why I suggested to hold the GURL instance itself, I think it would reduce
the number of conversions between GURL<->string as well.

https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
File ui/message_center/views/constants.h (left):

https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
ui/message_center/views/constants.h:24: const int kTextLeftPadding = kIconSize +
message_center::kIconToTextPadding;
On 2015/08/14 18:28:45, Miguel Garcia wrote:
> On 2015/08/14 17:17:07, Jun Mukai wrote:
> > Please do not move views specific constants to generic
message_center_style.h
> 
> Ok, any advice on where to put them so they can be reshared in mac and all
other
> platforms?

If they are shared among multiple platforms,
ui/message_center/message_center_style.h is the place to put. Sorry, I'm missing
something, where is the place to use these variables outside of the views/ code?

[chromium-reviews, 2015-08-17 18:44:15]

I checked with the ui/ owners (sadrul in particular) and the confirmed that
components is the way to go. I will add him as a deps owner on the review.

I cannot specify the elided url in the controller
(platform_notification_service_impl) because I dontbhave access to the
pixel size fonts etc. So it is not possible to figure out how much space we
have. It also seems that different platforms use different parameters so it
is better left as a view decision.

Will move the constants to ui/message_center/message_center_style.h.

Thanks! It seems that in spite of the large change this is the way to go.
On 17 Aug 2015 7:36 pm, <mukai@chromium.org> wrote:

> Also please consider the Ryan's comment in
> https://chromiumcodereview.appspot.com/1235253009/.
>
> You may want to move url_formatter into ui (under ui/gfx for example), or
> the
> url_formatting should be done in somewhere else.
>
> Another design suggestion -- do nothing for message_center side, but
> specify the
> elided URL as the context message in platform_notification_service_impl.cc?
>
> That way, the dependency confusion doesn't happen, and the change would be
> slightly simpler.
>
>
>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
> File ui/message_center/notification.h (right):
>
>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
> ui/message_center/notification.h:36: struct MESSAGE_CENTER_EXPORT
> ContextMessage {
> On 2015/08/14 18:28:45, Miguel Garcia wrote:
>
>> On 2015/08/14 17:17:07, Jun Mukai wrote:
>> > I prefer to keep string16 for context message. Could you:
>> > - move the 'origin_url' field from
>>
> chrome/browser/notifications/notification.h
>
>> > to ui/message_center/notification.h
>> > - use this origin_url if context_message is empty (and origin_url
>>
> isn't
>
>> > extension origin), apply the url_formatter to the origin_url
>>
>
> Will give a shot, now that you mention it I recall Peter saying
>>
> exactly the same
>
>> thing about not switching to std::string but I cannot remeber the
>>
> reasoning,
>
>> could you elaborate why not make the change?
>>
>
> string16 is the text representation on UI side, so it's a good design to
> convert into string16 at the earliest point and keep it as far as
> possible -- also it will reduce the number of conversions / copies.
>
> I think, however, in your new design, this might not satisfy because you
> want to the message into a GURL anyways.
> That's why I suggested to hold the GURL instance itself, I think it
> would reduce the number of conversions between GURL<->string as well.
>
>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
> File ui/message_center/views/constants.h (left):
>
>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
> ui/message_center/views/constants.h:24: const int kTextLeftPadding =
> kIconSize + message_center::kIconToTextPadding;
> On 2015/08/14 18:28:45, Miguel Garcia wrote:
>
>> On 2015/08/14 17:17:07, Jun Mukai wrote:
>> > Please do not move views specific constants to generic
>>
> message_center_style.h
>
> Ok, any advice on where to put them so they can be reshared in mac and
>>
> all other
>
>> platforms?
>>
>
> If they are shared among multiple platforms,
> ui/message_center/message_center_style.h is the place to put. Sorry, I'm
> missing something, where is the place to use these variables outside of
> the views/ code?
>
> https://chromiumcodereview.appspot.com/1292003004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2015-08-17 18:44:47]

On 17 Aug 2015 7:44 pm, "Miguel Garcia" <miguelg@google.com> wrote:

> I checked with the ui/ owners (sadrul in particular) and the confirmed
> that components is the way to go. I will add him as a deps owner on the
> review.
>
> I cannot specify the elided url in the controller
> (platform_notification_service_impl) because I dontbhave access to the
> pixel size fonts etc. So it is not possible to figure out how much space we
> have. It also seems that different platforms use different parameters so it
> is better left as a view decision.
>
> Will move the constants to ui/message_center/message_center_style.h.
>
> Thanks! It seems that in spite of the large change this is the way to go.
> On 17 Aug 2015 7:36 pm, <mukai@chromium.org> wrote:
>
>> Also please consider the Ryan's comment in
>> https://chromiumcodereview.appspot.com/1235253009/.
>>
>> You may want to move url_formatter into ui (under ui/gfx for example), or
>> the
>> url_formatting should be done in somewhere else.
>>
>> Another design suggestion -- do nothing for message_center side, but
>> specify the
>> elided URL as the context message in
>> platform_notification_service_impl.cc?
>>
>> That way, the dependency confusion doesn't happen, and the change would be
>> slightly simpler.
>>
>>
>>
>>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
>> File ui/message_center/notification.h (right):
>>
>>
>>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
>> ui/message_center/notification.h:36: struct MESSAGE_CENTER_EXPORT
>> ContextMessage {
>> On 2015/08/14 18:28:45, Miguel Garcia wrote:
>>
>>> On 2015/08/14 17:17:07, Jun Mukai wrote:
>>> > I prefer to keep string16 for context message. Could you:
>>> > - move the 'origin_url' field from
>>>
>> chrome/browser/notifications/notification.h
>>
>>> > to ui/message_center/notification.h
>>> > - use this origin_url if context_message is empty (and origin_url
>>>
>> isn't
>>
>>> > extension origin), apply the url_formatter to the origin_url
>>>
>>
>> Will give a shot, now that you mention it I recall Peter saying
>>>
>> exactly the same
>>
>>> thing about not switching to std::string but I cannot remeber the
>>>
>> reasoning,
>>
>>> could you elaborate why not make the change?
>>>
>>
>> string16 is the text representation on UI side, so it's a good design to
>> convert into string16 at the earliest point and keep it as far as
>> possible -- also it will reduce the number of conversions / copies.
>>
>> I think, however, in your new design, this might not satisfy because you
>> want to the message into a GURL anyways.
>> That's why I suggested to hold the GURL instance itself, I think it
>> would reduce the number of conversions between GURL<->string as well.
>>
>>
>>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
>> File ui/message_center/views/constants.h (left):
>>
>>
>>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
>> ui/message_center/views/constants.h:24: const int kTextLeftPadding =
>> kIconSize + message_center::kIconToTextPadding;
>> On 2015/08/14 18:28:45, Miguel Garcia wrote:
>>
>>> On 2015/08/14 17:17:07, Jun Mukai wrote:
>>> > Please do not move views specific constants to generic
>>>
>> message_center_style.h
>>
>> Ok, any advice on where to put them so they can be reshared in mac and
>>>
>> all other
>>
>>> platforms?
>>>
>>
>> If they are shared among multiple platforms,
>> ui/message_center/message_center_style.h is the place to put. Sorry, I'm
>> missing something, where is the place to use these variables outside of
>> the views/ code?
>>
>> https://chromiumcodereview.appspot.com/1292003004/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2015-08-18 16:04:13]

Ok, new change uploaded. PTAL

Here is a summary of the different open points we had

Re: android. eliding url does not work there, we are going to need a java
version of ElideHost so for now we are only eliding on desktop

While it makes it a large patch, your suggestion of moving origin_url to
message_center::Notification makes sense, the only files that have real code are
the ones you have reviewed already, everything else just changes the constructor
call of notifications everywhere.

The constants in message_center_style.h are used by mac (in
ui/message_center/cocoa/notification_controller.mm) and linux/win/etc (in
ui/message_center/views/notification_view.cc)

Thanks a lot for the timing reviews and for helping me converge to a solution!


On 2015/08/17 18:44:47, Miguel Garcia wrote:
> On 17 Aug 2015 7:44 pm, "Miguel Garcia" <mailto:miguelg@google.com> wrote:
> 
> > I checked with the ui/ owners (sadrul in particular) and the confirmed
> > that components is the way to go. I will add him as a deps owner on the
> > review.
> >
> > I cannot specify the elided url in the controller
> > (platform_notification_service_impl) because I dontbhave access to the
> > pixel size fonts etc. So it is not possible to figure out how much space we
> > have. It also seems that different platforms use different parameters so it
> > is better left as a view decision.
> >
> > Will move the constants to ui/message_center/message_center_style.h.
> >
> > Thanks! It seems that in spite of the large change this is the way to go.
> > On 17 Aug 2015 7:36 pm, <mailto:mukai@chromium.org> wrote:
> >
> >> Also please consider the Ryan's comment in
> >> https://chromiumcodereview.appspot.com/1235253009/.
> >>
> >> You may want to move url_formatter into ui (under ui/gfx for example), or
> >> the
> >> url_formatting should be done in somewhere else.
> >>
> >> Another design suggestion -- do nothing for message_center side, but
> >> specify the
> >> elided URL as the context message in
> >> platform_notification_service_impl.cc?
> >>
> >> That way, the dependency confusion doesn't happen, and the change would be
> >> slightly simpler.
> >>
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
> >> File ui/message_center/notification.h (right):
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/no...
> >> ui/message_center/notification.h:36: struct MESSAGE_CENTER_EXPORT
> >> ContextMessage {
> >> On 2015/08/14 18:28:45, Miguel Garcia wrote:
> >>
> >>> On 2015/08/14 17:17:07, Jun Mukai wrote:
> >>> > I prefer to keep string16 for context message. Could you:
> >>> > - move the 'origin_url' field from
> >>>
> >> chrome/browser/notifications/notification.h
> >>
> >>> > to ui/message_center/notification.h
> >>> > - use this origin_url if context_message is empty (and origin_url
> >>>
> >> isn't
> >>
> >>> > extension origin), apply the url_formatter to the origin_url
> >>>
> >>
> >> Will give a shot, now that you mention it I recall Peter saying
> >>>
> >> exactly the same
> >>
> >>> thing about not switching to std::string but I cannot remeber the
> >>>
> >> reasoning,
> >>
> >>> could you elaborate why not make the change?
> >>>
> >>
> >> string16 is the text representation on UI side, so it's a good design to
> >> convert into string16 at the earliest point and keep it as far as
> >> possible -- also it will reduce the number of conversions / copies.
> >>
> >> I think, however, in your new design, this might not satisfy because you
> >> want to the message into a GURL anyways.
> >> That's why I suggested to hold the GURL instance itself, I think it
> >> would reduce the number of conversions between GURL<->string as well.
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
> >> File ui/message_center/views/constants.h (left):
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/vi...
> >> ui/message_center/views/constants.h:24: const int kTextLeftPadding =
> >> kIconSize + message_center::kIconToTextPadding;
> >> On 2015/08/14 18:28:45, Miguel Garcia wrote:
> >>
> >>> On 2015/08/14 17:17:07, Jun Mukai wrote:
> >>> > Please do not move views specific constants to generic
> >>>
> >> message_center_style.h
> >>
> >> Ok, any advice on where to put them so they can be reshared in mac and
> >>>
> >> all other
> >>
> >>> platforms?
> >>>
> >>
> >> If they are shared among multiple platforms,
> >> ui/message_center/message_center_style.h is the place to put. Sorry, I'm
> >> missing something, where is the place to use these variables outside of
> >> the views/ code?
> >>
> >> https://chromiumcodereview.appspot.com/1292003004/
> >>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2015-08-18 16:04:27]

https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/DEPS
File ui/message_center/DEPS (right):

https://chromiumcodereview.appspot.com/1292003004/diff/1/ui/message_center/DE...
ui/message_center/DEPS:2: "+components/url_formatter/elide_url.h",
On 2015/08/14 17:17:07, Jun Mukai wrote:
> I think it's okay to add the entire "components/url_formatter".

Done.

[Jun Mukai, 2015-08-18 18:28:48]

https://chromiumcodereview.appspot.com/1292003004/diff/20001/ui/message_cente...
File ui/message_center/notification.h (right):

https://chromiumcodereview.appspot.com/1292003004/diff/20001/ui/message_cente...
ui/message_center/notification.h:41: bool use_origin_as_context;
I really want to eliminate this flag (and the struct), I want to keep
|context_message| as a simple string16 if possible.
So, question -- when is this flag required?  I mean, which notification wants to
have empty context_message but don't want to show the origin URL?

One thing I noticed is the extensions. So question here is how we can suppress
the context message for extensions?  A few ideas I have:
- specifying invisible text (whitespace, " ") in notification_api.cc: ugly, but
working well
- allowing only certain scheme origins (file, http, and https) for the context
message: special case rules isn't very preferable, but less ugly
- specifying meaningful default context message in notification_api.cc (the
extension's name): clean, but changes the UX

[Miguel Garcia, 2015-08-19 15:02:59]

https://chromiumcodereview.appspot.com/1292003004/diff/20001/ui/message_cente...
File ui/message_center/notification.h (right):

https://chromiumcodereview.appspot.com/1292003004/diff/20001/ui/message_cente...
ui/message_center/notification.h:41: bool use_origin_as_context;
Following our chat yesterday I implemented the change you suggested

So now we display the origin iff
- context_message is empty
- and origin_url is valid
- and origin_url is http or https

That should cover everything and I added tests to enforce this behavior.

PTAL

On 2015/08/18 18:28:48, Jun Mukai wrote:
> I really want to eliminate this flag (and the struct), I want to keep
> |context_message| as a simple string16 if possible.
> So, question -- when is this flag required?  I mean, which notification wants
to
> have empty context_message but don't want to show the origin URL?
> 
> One thing I noticed is the extensions. So question here is how we can suppress
> the context message for extensions?  A few ideas I have:
> - specifying invisible text (whitespace, " ") in notification_api.cc: ugly,
but
> working well
> - allowing only certain scheme origins (file, http, and https) for the context
> message: special case rules isn't very preferable, but less ugly
> - specifying meaningful default context message in notification_api.cc (the
> extension's name): clean, but changes the UX

[Jun Mukai, 2015-08-19 17:26:05]

lgtm

You'd better get a style-check for objc files though. I'm not familiar with
Chromium's objc coding.


Also, I think you could omit the constructor parameter of origin_url from
ui/message_center/notification.h.  Instead, add a protected set_origin_url()
method and invoke it from the constructors of
chrome/browser/notifications/notification.cc

This idea is just to avoid asking too many owner approvals though.

[Jun Mukai, 2015-08-19 17:28:36]

On 2015/08/19 17:26:05, Jun Mukai wrote:
> lgtm
> 
> You'd better get a style-check for objc files though. I'm not familiar with
> Chromium's objc coding.

I mean, someone familiar with objc code, such like rsesek@ or thakis@.
They originally wrote those ui/message_center/cocoa files.

[Miguel Garcia, 2015-08-19 17:50:23]

miguelg@chromium.org changed reviewers:
+ oshima@chromium.org, thakis@chromium.org

[Miguel Garcia, 2015-08-19 17:50:23]

Thanks!

 I rather keep the constructor, since it's such a mechanic change on those files
I think that if approvals get delayed I can just TBR

+thakis@ Can you have a look at the obj-c++ code? First time I ever touch mac
code so please be thorough.
+oshima@ for OWNERS in ash/ and chrome/browser/chromeos

[Nico, 2015-08-19 17:58:12]

Objective-C looks fine.

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
File ui/message_center/cocoa/notification_controller.h (right):

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
ui/message_center/cocoa/notification_controller.h:74: -
(bool)useOriginAsContextMessage:
https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/Progra...
" The exception to this rule is for Boolean properties, for which the getter
method should start with is" (also, return BOOL not bool)

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
File ui/message_center/cocoa/notification_controller.mm (right):

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
ui/message_center/cocoa/notification_controller.mm:388:
actualLines:&actualTitleLines])];
Huh, is this what clang-format does? The formatting on the left looks more like
how this usually looks. But if this is clang-format I suppose the new formatting
is fine too.

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
ui/message_center/cocoa/notification_controller.mm:449:
maxNumberOfLines:message_center::kContextMessageLineLimit])];
This might look a bit less awkward with a temp variable.

(base::string16 elided = [self wrapText:message forFont:[contextMessage_ font];
 [contextMessage_ setString:base::Sys...)

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
ui/message_center/cocoa/notification_controller.mm:665:
notification->origin_url().SchemeIsHTTPOrHTTPS());
This kind of looks like model code; why isn't this code in a method on
Notification?

[Jun Mukai, 2015-08-19 18:06:02]

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
File ui/message_center/cocoa/notification_controller.mm (right):

https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
ui/message_center/cocoa/notification_controller.mm:665:
notification->origin_url().SchemeIsHTTPOrHTTPS());
On 2015/08/19 17:58:12, Nico wrote:
> This kind of looks like model code; why isn't this code in a method on
> Notification?

+1 (to be clear, I objected the idea to introduce a boolean flag for this, but
adding a method like this looks good to me).

[Miguel Garcia, 2015-08-19 18:07:00]

Thanks for the review!

On 2015/08/19 17:58:12, Nico wrote:
> Objective-C looks fine.
> 
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> File ui/message_center/cocoa/notification_controller.h (right):
> 
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> ui/message_center/cocoa/notification_controller.h:74: -
> (bool)useOriginAsContextMessage:
>
https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/Progra...
> " The exception to this rule is for Boolean properties, for which the getter
> method should start with is" (also, return BOOL not bool)
> 
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> File ui/message_center/cocoa/notification_controller.mm (right):
> 
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> ui/message_center/cocoa/notification_controller.mm:388:
> actualLines:&actualTitleLines])];
> Huh, is this what clang-format does? The formatting on the left looks more
like
> how this usually looks. But if this is clang-format I suppose the new
formatting
> is fine too.
> 
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> ui/message_center/cocoa/notification_controller.mm:449:
> maxNumberOfLines:message_center::kContextMessageLineLimit])];
> This might look a bit less awkward with a temp variable.
> 
> (base::string16 elided = [self wrapText:message forFont:[contextMessage_
font];
>  [contextMessage_ setString:base::Sys...)
> 
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> ui/message_center/cocoa/notification_controller.mm:665:
> notification->origin_url().SchemeIsHTTPOrHTTPS());
> This kind of looks like model code; why isn't this code in a method on
> Notification?

Funny, I did exactly that in patch 2, if you look at the code there the decision
was driven by a boolean passed in the notification class. Jun and I chatted a
bit about it and decided to go this way since it matches what the OWNERS prefer.
In a nutshell, notification being a bag of static data instead of a controller
(in the MVP sense) and the decision of what to paint or not is done in view.

[Miguel Garcia, 2015-08-19 18:09:07]

Just saw Jun's reply, it seems we all agree now! I will add the method to
notification then.

On 2015/08/19 18:07:00, Miguel Garcia wrote:
> Thanks for the review!
> 
> On 2015/08/19 17:58:12, Nico wrote:
> > Objective-C looks fine.
> > 
> >
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> > File ui/message_center/cocoa/notification_controller.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> > ui/message_center/cocoa/notification_controller.h:74: -
> > (bool)useOriginAsContextMessage:
> >
>
https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/Progra...
> > " The exception to this rule is for Boolean properties, for which the getter
> > method should start with is" (also, return BOOL not bool)
> > 
> >
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> > File ui/message_center/cocoa/notification_controller.mm (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> > ui/message_center/cocoa/notification_controller.mm:388:
> > actualLines:&actualTitleLines])];
> > Huh, is this what clang-format does? The formatting on the left looks more
> like
> > how this usually looks. But if this is clang-format I suppose the new
> formatting
> > is fine too.
> > 
> >
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> > ui/message_center/cocoa/notification_controller.mm:449:
> > maxNumberOfLines:message_center::kContextMessageLineLimit])];
> > This might look a bit less awkward with a temp variable.
> > 
> > (base::string16 elided = [self wrapText:message forFont:[contextMessage_
> font];
> >  [contextMessage_ setString:base::Sys...)
> > 
> >
>
https://chromiumcodereview.appspot.com/1292003004/diff/40001/ui/message_cente...
> > ui/message_center/cocoa/notification_controller.mm:665:
> > notification->origin_url().SchemeIsHTTPOrHTTPS());
> > This kind of looks like model code; why isn't this code in a method on
> > Notification?
> 
> Funny, I did exactly that in patch 2, if you look at the code there the
decision
> was driven by a boolean passed in the notification class. Jun and I chatted a
> bit about it and decided to go this way since it matches what the OWNERS
prefer.
> In a nutshell, notification being a bag of static data instead of a controller
> (in the MVP sense) and the decision of what to paint or not is done in view.

[Miguel Garcia, 2015-08-19 19:52:46]

PTAL

https://codereview.chromium.org/1292003004/diff/40001/ui/message_center/cocoa...
File ui/message_center/cocoa/notification_controller.h (right):

https://codereview.chromium.org/1292003004/diff/40001/ui/message_center/cocoa...
ui/message_center/cocoa/notification_controller.h:74: -
(bool)useOriginAsContextMessage:
On 2015/08/19 17:58:11, Nico wrote:
>
https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/Progra...
> " The exception to this rule is for Boolean properties, for which the getter
> method should start with is" (also, return BOOL not bool)

Acknowledged.

https://codereview.chromium.org/1292003004/diff/40001/ui/message_center/cocoa...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/1292003004/diff/40001/ui/message_center/cocoa...
ui/message_center/cocoa/notification_controller.mm:388:
actualLines:&actualTitleLines])];
On 2015/08/19 17:58:12, Nico wrote:
> Huh, is this what clang-format does? The formatting on the left looks more
like
> how this usually looks. But if this is clang-format I suppose the new
formatting
> is fine too.

Acknowledged.

https://codereview.chromium.org/1292003004/diff/40001/ui/message_center/cocoa...
ui/message_center/cocoa/notification_controller.mm:449:
maxNumberOfLines:message_center::kContextMessageLineLimit])];
On 2015/08/19 17:58:11, Nico wrote:
> This might look a bit less awkward with a temp variable.
> 
> (base::string16 elided = [self wrapText:message forFont:[contextMessage_
font];
>  [contextMessage_ setString:base::Sys...)

Done.

https://codereview.chromium.org/1292003004/diff/40001/ui/message_center/cocoa...
ui/message_center/cocoa/notification_controller.mm:665:
notification->origin_url().SchemeIsHTTPOrHTTPS());
Done

On 2015/08/19 18:06:02, Jun Mukai wrote:
> On 2015/08/19 17:58:12, Nico wrote:
> > This kind of looks like model code; why isn't this code in a method on
> > Notification?
> 
> +1 (to be clear, I objected the idea to introduce a boolean flag for this, but
> adding a method like this looks good to me).

[Nico, 2015-08-19 21:05:31]

lgtm with formatting nit addressed

https://codereview.chromium.org/1292003004/diff/60001/ui/message_center/cocoa...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/1292003004/diff/60001/ui/message_center/cocoa...
ui/message_center/cocoa/notification_controller.mm:446: forFont:[contextMessage_
font]
align the lines on the :s (`git cl format` should do this)

[oshima, 2015-08-20 00:32:35]

ash, c/b/chromeos lgtm

[Miguel Garcia, 2015-08-20 07:44:54]

TBR pkasting for the new DEPS.

[Miguel Garcia, 2015-08-20 07:45:03]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2015-08-20 07:45:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mukai@chromium.org,
oshima@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1292003004/#ps80001
(title: "Style nit")

[commit-bot: I haz the power, 2015-08-20 07:45:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292003004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292003004/80001

[Miguel Garcia, 2015-08-20 07:45:26]

https://codereview.chromium.org/1292003004/diff/60001/ui/message_center/cocoa...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/1292003004/diff/60001/ui/message_center/cocoa...
ui/message_center/cocoa/notification_controller.mm:446: forFont:[contextMessage_
font]
On 2015/08/19 21:05:30, Nico (on vacation Thu Aug 20) wrote:
> align the lines on the :s (`git cl format` should do this)

Done.

[Peter Kasting, 2015-08-20 08:09:13]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-08-20 08:09:14]

LGTM

[commit-bot: I haz the power, 2015-08-20 08:17:22]

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

[commit-bot: I haz the power, 2015-08-20 08:17:23]

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

[Miguel Garcia, 2015-08-20 08:24:51]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2015-08-20 08:25:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292003004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292003004/80001

[commit-bot: I haz the power, 2015-08-20 09:00:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-20 09:01:29]

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