Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade from Chromium 80.0.3987.66 to Chromium 80.0.3987.78. #4464

Merged
merged 4 commits into from
Feb 3, 2020

Conversation

mkarolin
Copy link
Collaborator

@mkarolin mkarolin commented Jan 30, 2020

Fixes brave/brave-browser#7975
Related PR: brave/brave-browser#7979

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

Type ANNOUNCEMENT has been added.

Chromium change:

commit 147807796e2bbd28853c41166a5611c2fd294d28
Author: Xing Liu <[email protected]>
Date:   Sat Jan 25 03:08:30 2020 +0000

    Announcement notification for M80.

    This CL finishes the implementation of announcement notification. This
    feature targets M80.

    We show a notification on browser start up on Android and desktops for
    an important informational update.

    [email protected], [email protected], [email protected]

    Bug: 1042124
@mkarolin mkarolin added this to the 1.5.x - Nightly milestone Jan 30, 2020
@mkarolin mkarolin self-assigned this Jan 30, 2020
@mkarolin mkarolin marked this pull request as ready for review January 30, 2020 00:53
Build error on CI for Android:

20:26:04  In file included from ../../chrome/browser/updates/announcement_notification/announcement_notification_service.cc:17:
20:26:04  ../../brave/chromium_src/chrome/grit/generated_resources.h:9:10: fatal error: 'brave/grit/brave_generated_resources.h' file not found
20:26:04  #include "brave/grit/brave_generated_resources.h"
20:26:04           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
20:26:04  1 error generated.

Looks like a Chromium bug. Chromium change is:

https://chromium.googlesource.com/chromium/src/+/77c4d4c42f406fe1541fdd145b8201ab78ff9651

commit 77c4d4c42f406fe1541fdd145b8201ab78ff9651
Author: Xing Liu <[email protected]>
Date:   Tue Jan 28 03:58:56 2020 +0000

    Announcement notification: Fix navigation to URL.

    Currently when clicking on the "Review" button, on Mac, it will always
    not navigate to the expected URL. On Debian, sometimes nothing happens.

    On Mac, if the notification has no origin, then click event will not be
    forwarded to NotificationHandler. This CL removes the origin check.

    Also changed the URL navigation API used to open the URL.

    Bug: 1042124

Specifically in chrome/browser/updates/announcement_notification/announcement_notification_service.cc:

@@ -14,8 +14,10 @@
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/updates/announcement_notification/announcement_notification_metrics.h"
+#include "chrome/grit/generated_resources.h"

but the BUILD.gn file was not updated with the dependency on generated
resources.
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ LGTM (pending CI)

@mkarolin can you rebase and push (to start CI again)? 😄

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++


deps = [
"//base",
+ "//chrome/app:generated_resources",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this looks like upstream bug w/o this for android build.
I think their buildbot didn't see this error by luck? :)

@simonhong
Copy link
Member

CI only has network-audit failure - maybe it's Crowd Deny.

@mkarolin
Copy link
Collaborator Author

mkarolin commented Feb 3, 2020

Yes, the CI failure in network-audit is consistent with Crowd Deny (issue brave/brave-browser#7977).

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please file an upstream bug for 672802d

@mkarolin mkarolin merged commit 6427405 into master Feb 3, 2020
@mkarolin mkarolin deleted the 80.0.3987.78 branch February 3, 2020 17:38
mkarolin added a commit that referenced this pull request Feb 3, 2020
Upgrade from Chromium 80.0.3987.66 to Chromium 80.0.3987.78.
mkarolin added a commit that referenced this pull request Feb 3, 2020
Upgrade from Chromium 80.0.3987.66 to Chromium 80.0.3987.78.
@simonhong
Copy link
Member

FYI, filed an issue for above deps issue - https://bugs.chromium.org/p/chromium/issues/detail?id=1048346.

@simonhong
Copy link
Member

FYI above deps issue was fixed in upstream - https://chromium-review.googlesource.com/c/chromium/src/+/2036915

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade from Chromium 80.0.3987.66 to Chromium 80.0.3987.78
4 participants