New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 762900 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Task

Blocking:
issue 762093



Sign in to add a comment

Don't use scheme in site notification channel names

Project Member Reported by awdf@chromium.org, Sep 7 2017

Issue description

Can pass a boolean when getting origin for display so scheme is not included.
 

Comment 2 by awdf@chromium.org, Sep 8 2017

Status: Fixed (was: Started)

Comment 3 by awdf@chromium.org, Sep 11 2017

Labels: -Pri-2 Merge-Request-62 M-62 Pri-1
Status: Started (was: Fixed)

Comment 4 by awdf@chromium.org, Sep 11 2017

Merge requesting for 62 because:

- 62 is the first version with site-specific channels in it
- Channel names are set when permission is first granted/blocked
- We don't want users to end up with some channel names that start with 'https' and some that don't.
- This is a one-line change (besides amending test expectations) that just affects how a string is displayed, so we're confident this is safe.
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 12 2017

Labels: -Merge-Request-62 Hotlist-Merge-Approved Merge-Approved-62
Your change meets the bar and is auto-approved for M62. Please go ahead and merge the CL to branch 3202 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 12 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a436a9089a84fa094688dfe9a84b2e5f27046c1

commit 1a436a9089a84fa094688dfe9a84b2e5f27046c1
Author: Anita Woodruff <awdf@chromium.org>
Date: Tue Sep 12 17:32:09 2017

[Android O Notifications] Strip scheme from site channel names

- Since notification permission can now only be granted to https sites,
the scheme is redundant.

TBR=awdf@chromium.org

(cherry picked from commit d01cae4f4597662544a4c406b3a5f0f42e51242d)

Bug:  762900 
Change-Id: I47c15c9ac71af7decab90d6ba781126a47f55b33
Reviewed-on: https://chromium-review.googlesource.com/655080
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#500321}
Reviewed-on: https://chromium-review.googlesource.com/663897
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#171}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/1a436a9089a84fa094688dfe9a84b2e5f27046c1/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java
[modify] https://crrev.com/1a436a9089a84fa094688dfe9a84b2e5f27046c1/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java
[modify] https://crrev.com/1a436a9089a84fa094688dfe9a84b2e5f27046c1/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java
[modify] https://crrev.com/1a436a9089a84fa094688dfe9a84b2e5f27046c1/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/SiteChannelsManagerTest.java

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66bef2ea0c4e861d6deba22a346bd0280995f50b

commit 66bef2ea0c4e861d6deba22a346bd0280995f50b
Author: Theresa <twellington@chromium.org>
Date: Wed Sep 13 15:19:47 2017

Revert "[Android O Notifications] Strip scheme from site channel names"

This reverts commit 1a436a9089a84fa094688dfe9a84b2e5f27046c1.

Reason for revert: Reason for revert: official builds failing to compile, crbug.com/764600

Original change's description:
> [Android O Notifications] Strip scheme from site channel names
> 
> - Since notification permission can now only be granted to https sites,
> the scheme is redundant.
> 
> TBR=awdf@chromium.org
> 
> (cherry picked from commit d01cae4f4597662544a4c406b3a5f0f42e51242d)
> 
> Bug:  762900 
> Change-Id: I47c15c9ac71af7decab90d6ba781126a47f55b33
> Reviewed-on: https://chromium-review.googlesource.com/655080
> Commit-Queue: Anita Woodruff <awdf@chromium.org>
> Reviewed-by: Peter Beverloo <peter@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#500321}
> Reviewed-on: https://chromium-review.googlesource.com/663897
> Reviewed-by: Anita Woodruff <awdf@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3202@{#171}
> Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}

TBR=peter@chromium.org,awdf@chromium.org

Change-Id: Id299988ce15490e5a91a2dbc20da26e89c4cf515
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  762900 
Reviewed-on: https://chromium-review.googlesource.com/665218
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#199}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/66bef2ea0c4e861d6deba22a346bd0280995f50b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java
[modify] https://crrev.com/66bef2ea0c4e861d6deba22a346bd0280995f50b/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java
[modify] https://crrev.com/66bef2ea0c4e861d6deba22a346bd0280995f50b/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java
[modify] https://crrev.com/66bef2ea0c4e861d6deba22a346bd0280995f50b/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/SiteChannelsManagerTest.java

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f27ce7218752aca787b68dae09bd3e7e93bc6239

commit f27ce7218752aca787b68dae09bd3e7e93bc6239
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Sep 13 15:40:52 2017

[Android O Notifications] Strip scheme from site channel names

- Since notification permission can now only be granted to https sites,
the scheme is redundant.

- Re-cherry-picked: this time with the necessary imports.

TBR=peter@chromium.org

(cherry picked from commit d01cae4f4597662544a4c406b3a5f0f42e51242d)

Bug:  762900 
Change-Id: I7383d5ded63b19c73ae185ad8d6fa3af510d27f8
Reviewed-on: https://chromium-review.googlesource.com/655080
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#500321}
Reviewed-on: https://chromium-review.googlesource.com/664564
Reviewed-by: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#202}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/f27ce7218752aca787b68dae09bd3e7e93bc6239/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java
[modify] https://crrev.com/f27ce7218752aca787b68dae09bd3e7e93bc6239/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java
[modify] https://crrev.com/f27ce7218752aca787b68dae09bd3e7e93bc6239/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java
[modify] https://crrev.com/f27ce7218752aca787b68dae09bd3e7e93bc6239/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/SiteChannelsManagerTest.java

Comment 9 by awdf@chromium.org, Sep 14 2017

Status: Fixed (was: Started)

Sign in to add a comment