Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2508663002:
[ios] Move NativeAppLauncher upstream (Closed)
Description[ios] Move NativeAppLauncher upstream
BUG=662573
Committed: https://crrev.com/6db193bb9e47e9a6dc442e379f73441aca51a281
Cr-Commit-Position: refs/heads/master@{#433923}
Patch Set 1 : Upstream NativeAppLauncher Files #
Total comments: 25
Patch Set 2 : Unittests, CL Feedback #
Total comments: 6
Patch Set 3 : CL Feedback #
Total comments: 2
Patch Set 4 : String description change #
Total comments: 13
Patch Set 5 : CL Feedback #Messages
Total messages: 38 (25 generated)
Description was changed from ========== [ios] Move NativeAppLauncher upstream. (Still WIP) BUG=662573 ========== to ========== [ios] Move NativeAppLauncher upstream BUG=662573 ==========
sczs@google.com changed reviewers: + rohitrao@chromium.org
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sczs@google.com to run a CQ dry run
Hi Rohit, could you PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rohitrao@chromium.org changed reviewers: + sdefresne@chromium.org
+sylvain to review as well. What happens if we have duplicate strings upstream and downstream? How do we move strings in a way that avoids issues? https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/BUIL... File ios/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/BUIL... ios/chrome/browser/BUILD.gn:100: "//ios/chrome/browser/native_app_launcher", You'll need to remove this dependency for now, to avoid breaking the build during the roll. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:24: } Add the following to this file: group("unit_tests") { deps = [ ":native_app_launcher", ] } And then in ios/chrome/BUILD.gn, add a dependency to this "unit_tests" target to ios_chrome_unittests. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/ios_appstore_ids.h (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.h:8: namespace ios_internal { Remove the namespace. We're intentionally trying not to use namespaces anymore. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.h:10: // Constants with the iOS AppStore IDs for Google native iOS apps recognized // Appstore IDs. Let's drop the google-specific language from the comment. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.h:24: // Non-Google iOS native apps that we recognize. Recognizes TestFlight so Let's drop this comment entirely and merge this constant into the section above. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm:9: // Constants with the iOS AppStore IDs for Google native iOS apps recognized Remove this comment. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm:23: // AppStore ID for TestFlight. Remove this comment and merge the constant into the section above. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_controller.mm (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_controller.mm:37: viewForDelegate:(infobars::InfoBarDelegate*)delegate This should be indented 4 spaces. Did you run "git cl format"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:267: <message name="IDS_APP_LAUNCHER_OPEN_ONCE_BUTTON_IOS" desc="Label of a button to choose to launch an application once, as opposed to always. [Length: 12em] [iOS only]"> The strings identifier should be changed from IDS_FOO to IDS_IOS_FOO to: 1. make it obvious they are iOS only when looking at the code without checking in which file they are defined, 2. avoid collisions with the same strings defined downstream (at the point of upstreaming, later the downstream version should be removed), 3. keep consistent with the rest of the file. To answer rohitrao question, for upstreaming strings, we change id, upstream them, change code downstream to use the new version, and then remove the downstream string. This can be done in two CLs (one for landing the strings upstream, one to use them downstream and remove the old strings). The same process can be used for other resources (images, ...). https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm:23: // AppStore ID for TestFlight. On 2016/11/17 03:15:22, rohitrao wrote: > Remove this comment and merge the constant into the section above. when merging, keep name in alphabetic order. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_controller.mm (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_controller.mm:37: viewForDelegate:(infobars::InfoBarDelegate*)delegate On 2016/11/17 03:15:23, rohitrao wrote: > This should be indented 4 spaces. Did you run "git cl format"? Not sure this should be indented by 4 spaces as this is the method name, not the name of a parameter. I think it is formatted the same as a long method name with a long return type in C++ (but maybe this is a bug of clang-format). https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:15: class GURL; Remove either this forward-declaration or the include of url/gurl.h. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:22: } For consistency, add "// namespace infobars" here like is done for the other namespace (or remove it since it is a namespace of just one line. Please add blank lines before and after each namespaces. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:61: virtual void UserPerformedAction(NativeAppActionType userAction); Where are the tests? Can they also be upstreamed?
The CQ bit was checked by sczs@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. This should be ready to land. It compiles without the downstream part, so it won't break the roller. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:267: <message name="IDS_APP_LAUNCHER_OPEN_ONCE_BUTTON_IOS" desc="Label of a button to choose to launch an application once, as opposed to always. [Length: 12em] [iOS only]"> On 2016/11/17 06:24:26, sdefresne wrote: > The strings identifier should be changed from IDS_FOO to IDS_IOS_FOO to: > 1. make it obvious they are iOS only when looking at the code without checking > in which file they are defined, > 2. avoid collisions with the same strings defined downstream (at the point of > upstreaming, later the downstream version should be removed), > 3. keep consistent with the rest of the file. > > To answer rohitrao question, for upstreaming strings, we change id, upstream > them, change code downstream to use the new version, and then remove the > downstream string. This can be done in two CLs (one for landing the strings > upstream, one to use them downstream and remove the old strings). > > The same process can be used for other resources (images, ...). Thanks for the detailed explanation. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:24: } On 2016/11/17 03:15:22, rohitrao wrote: > Add the following to this file: > > group("unit_tests") { > deps = [ > ":native_app_launcher", > ] > } > > And then in ios/chrome/BUILD.gn, add a dependency to this "unit_tests" target to > ios_chrome_unittests. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/ios_appstore_ids.h (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.h:8: namespace ios_internal { On 2016/11/17 03:15:22, rohitrao wrote: > Remove the namespace. We're intentionally trying not to use namespaces anymore. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.h:10: // Constants with the iOS AppStore IDs for Google native iOS apps recognized On 2016/11/17 03:15:22, rohitrao wrote: > // Appstore IDs. > > Let's drop the google-specific language from the comment. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.h:24: // Non-Google iOS native apps that we recognize. Recognizes TestFlight so On 2016/11/17 03:15:22, rohitrao wrote: > Let's drop this comment entirely and merge this constant into the section above. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm:9: // Constants with the iOS AppStore IDs for Google native iOS apps recognized On 2016/11/17 03:15:22, rohitrao wrote: > Remove this comment. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/ios_appstore_ids.mm:23: // AppStore ID for TestFlight. On 2016/11/17 06:24:26, sdefresne wrote: > On 2016/11/17 03:15:22, rohitrao wrote: > > Remove this comment and merge the constant into the section above. > > when merging, keep name in alphabetic order. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_controller.mm (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_controller.mm:37: viewForDelegate:(infobars::InfoBarDelegate*)delegate On 2016/11/17 06:24:26, sdefresne wrote: > On 2016/11/17 03:15:23, rohitrao wrote: > > This should be indented 4 spaces. Did you run "git cl format"? > > Not sure this should be indented by 4 spaces as this is the method name, not the > name of a parameter. I think it is formatted the same as a long method name with > a long return type in C++ (but maybe this is a bug of clang-format). I ran git cl format and it formatted these files. I was avoiding that but I got a presubmit warning that some files needed formatting. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h (right): https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:15: class GURL; On 2016/11/17 06:24:26, sdefresne wrote: > Remove either this forward-declaration or the include of url/gurl.h. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:22: } On 2016/11/17 06:24:26, sdefresne wrote: > For consistency, add "// namespace infobars" here like is done for the other > namespace (or remove it since it is a namespace of just one line. Please add > blank lines before and after each namespaces. Done. https://codereview.chromium.org/2508663002/diff/20001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:61: virtual void UserPerformedAction(NativeAppActionType userAction); On 2016/11/17 06:24:26, sdefresne wrote: > Where are the tests? Can they also be upstreamed? They have now been upstreamed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:21: "//ui/base:base", Just "//ui/base" https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:22: "//url:url", Just "//url" https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate_unittest.mm (right): https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate_unittest.mm:21: delegate_.reset(new NativeAppInfoBarDelegate( I think you don't need a class with a member variable, but instead you can just use a local variable: TEST(NativeAppInfoBarDelegateTest, TestConstructor) { std::unique_ptr<NativeAppInfoBarDelegate> delegate; delegate = base::MakeUnique<NativeAppInfoBarDelegate>( nil, GURL(), NATIVE_APP_INSTALLER_CONTROLLER); EXPECT_TRUE(delegate.get()); ... However I'm not sure what is the point of that test. It looks like it just create object and check that "new" returned a non-null pointer. However, "new" cannot return non-null pointer (if there is no memory available, a hook is called to release memory, and if it fails, either an exception is raised if the code is compiled with exception support or a program terminating function, usually abort, is called), thus the test is pointless. I'd recommend adding real tests and removing this one (if no other tests can be written, then just delete the file).
Implemented CL Feedback PTAL https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:21: "//ui/base:base", On 2016/11/21 08:46:04, sdefresne wrote: > Just "//ui/base" Done. https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:22: "//url:url", On 2016/11/21 08:46:04, sdefresne wrote: > Just "//url" Done. https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate_unittest.mm (right): https://codereview.chromium.org/2508663002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate_unittest.mm:21: delegate_.reset(new NativeAppInfoBarDelegate( On 2016/11/21 08:46:04, sdefresne wrote: > I think you don't need a class with a member variable, but instead you can just > use a local variable: > > TEST(NativeAppInfoBarDelegateTest, TestConstructor) { > std::unique_ptr<NativeAppInfoBarDelegate> delegate; > delegate = base::MakeUnique<NativeAppInfoBarDelegate>( > nil, GURL(), NATIVE_APP_INSTALLER_CONTROLLER); > EXPECT_TRUE(delegate.get()); > ... > > However I'm not sure what is the point of that test. It looks like it just > create object and check that "new" returned a non-null pointer. However, "new" > cannot return non-null pointer (if there is no memory available, a hook is > called to release memory, and if it fails, either an exception is raised if the > code is compiled with exception support or a program terminating function, > usually abort, is called), thus the test is pointless. > > I'd recommend adding real tests and removing this one (if no other tests can be > written, then just delete the file). Thanks Sylvain, I modified the test as you suggested and added some actual check for identifiers.
lgtm https://codereview.chromium.org/2508663002/diff/60001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2508663002/diff/60001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:276: <message name="IDS_IOS_APP_LAUNCHER_OPEN_IN_APP_QUESTION_MESSAGE" desc="Message in the infobar asking the user if they want to open the current page in a google app. [Length: 40em] [iOS only]"> Reword this description to say "in an external app".
The CQ bit was checked by sczs@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2508663002/diff/60001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2508663002/diff/60001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:276: <message name="IDS_IOS_APP_LAUNCHER_OPEN_IN_APP_QUESTION_MESSAGE" desc="Message in the infobar asking the user if they want to open the current page in a google app. [Length: 40em] [iOS only]"> On 2016/11/21 19:16:03, rohitrao wrote: > Reword this description to say "in an external app". Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm once comments addressed https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:284: </message> nit: indentation is incorrect on this line https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h (right): https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:45: const GURL& pageURL, This is a c++ variable so it should use_cpp_naming_convention. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:69: GURL pageURL_; This is a c++ member variable, so it should use_cpp_naming_convention_. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm (right): https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm:27: : controller_(controller), pageURL_(pageURL), type_(type) {} ditto https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm:34: id<NativeAppNavigationControllerProtocol> controllerProtocol, This is a c++ variable, so it should use_cpp_naming_convention. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm:35: const GURL& pageURL, ditto https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm:38: std::unique_ptr<NativeAppInfoBarDelegate> delegate( Can you use base::MakeUnique<> here and below, this make it easier to read IMO? auto infobar = base::MakeUnique<InfoBarIOS>( base::MakeUnique<NativeAppInfoBarDelegate>( controller_protocol, page_url, type)); ...
The CQ bit was checked by sczs@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Testing the CL now https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:284: </message> On 2016/11/22 10:16:19, sdefresne wrote: > nit: indentation is incorrect on this line Done. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h (right): https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:45: const GURL& pageURL, On 2016/11/22 10:16:19, sdefresne wrote: > This is a c++ variable so it should use_cpp_naming_convention. Done. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h:69: GURL pageURL_; On 2016/11/22 10:16:19, sdefresne wrote: > This is a c++ member variable, so it should use_cpp_naming_convention_. Done. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm (right): https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm:27: : controller_(controller), pageURL_(pageURL), type_(type) {} On 2016/11/22 10:16:19, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm:34: id<NativeAppNavigationControllerProtocol> controllerProtocol, On 2016/11/22 10:16:19, sdefresne wrote: > This is a c++ variable, so it should use_cpp_naming_convention. Done. https://codereview.chromium.org/2508663002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.mm:35: const GURL& pageURL, On 2016/11/22 10:16:19, sdefresne wrote: > ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sczs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rohitrao@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2508663002/#ps100001 (title: "CL Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479839464591790,
"parent_rev": "d73c43910abe359ac1ee3162d82e80df4862c2dd", "commit_rev":
"74aba34a98179d758bc5399f52b40e54440da89c"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Move NativeAppLauncher upstream BUG=662573 ========== to ========== [ios] Move NativeAppLauncher upstream BUG=662573 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [ios] Move NativeAppLauncher upstream BUG=662573 ========== to ========== [ios] Move NativeAppLauncher upstream BUG=662573 Committed: https://crrev.com/6db193bb9e47e9a6dc442e379f73441aca51a281 Cr-Commit-Position: refs/heads/master@{#433923} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6db193bb9e47e9a6dc442e379f73441aca51a281 Cr-Commit-Position: refs/heads/master@{#433923} |
