New issue
Advanced search Search tips

Issue 798466 link

Starred by 1 user

Issue metadata

Status: Archived
Owner: ----
Closed: Sep 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocked on:
issue 763980



Sign in to add a comment

Use the right types for notification properties everywhere

Project Member Reported by awdf@chromium.org, Jan 2 2018

Issue description

Right now we use vectors of chars in blink + content for the notification.data field, but need to convert to vectors of ints in the new mojo code in https://crrev.com/c/832396 [1] because mojo has no char type.

There is no particular reason why this needs to be a vector of chars in the blink + content code, so we could instead align
by using vectors of uint8s everywhere from WebNotificationData.cpp onwards.

Doing this will simplify the code in the mojo struct trait conversions.

[1]  Issue 595685 
 

Comment 1 by awdf@chromium.org, Jan 2 2018

-Also, using uint8 is the standard way to represent binary data.

Note the field type in the notification database will also need to be updated (this requires changing the protobuf format)

Comment 2 by awdf@chromium.org, Jan 2 2018

Summary: Align types used for notification data and vibration pattern (was: Align types used for notification data)
Vibration pattern should also be unified to be a vector of int32s everywhere (right now we have to convert between ints & int32s)

Comment 3 by peter@chromium.org, Jan 2 2018

Also we should change PlatformNotificationAction::placeholder to be a base::Optional<base::string16>.

Comment 4 by awdf@chromium.org, Jan 3 2018

Cc: peter@chromium.org
 Issue 797306  has been merged into this issue.

Comment 5 by awdf@chromium.org, Jan 12 2018

** actually, we should use base::TimeDelta for the vibration pattern array elements (reason being it's always better to use more structured types where poss).

(See dcheng's comment on https://chromium-review.googlesource.com/c/chromium/src/+/832396/32/third_party/WebKit/public/platform/modules/notifications/notification.mojom )

Comment 6 by awdf@chromium.org, Jan 12 2018

Summary: Use the right types for notification properties everywhere (was: Align types used for notification data and vibration pattern)

Comment 7 by awdf@chromium.org, Jan 12 2018

... And we should use base::TimeTicks for timestamp too, according to dcheng@ , once https://crbug.com/763980 is done.

Comment 8 by awdf@chromium.org, Jan 12 2018

Blockedon: 763980

Comment 9 by awdf@chromium.org, Jan 17 2018

can also update our text direction enum with mojo.common.mojom.TextDirection
Status: Archived (was: Available)
Archiving old bugs that haven't been actively assigned in over 180 days.

If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!

Sign in to add a comment