Introduced StoreKitTabHelper class

Introduced StoreKitTabHelper class

Removed _tab from NativeAppNavigationController as everything can
be obtained from WebState.

BUG=684063
Review-Url: https://codereview.chromium.org/2731553005
Cr-Commit-Position: refs/heads/master@{#455460}
Committed: https://chromium.googlesource.com/chromium/src/+/d6e73e57a224c74a169abbcd8f2507fd1a5daa7b

[pkl (ping after 24h if needed), 2017-03-04 02:11:36]

pkl@chromium.org changed reviewers:
+ rohitrao@chromium.org, sdefresne@chromium.org

[pkl (ping after 24h if needed), 2017-03-04 02:11:37]

I introduced a new directory ios/chrome/browser/store_kit/ and
moved storekit_launch.h into it. This created quite a few file
changes because of the path to the .h file and also BUILD.gn
changes because of the new module.

When reviewing, the main changes are:
- Added StoreKitTabHelper class
- NativeAppNavigationController is now using WebState to access
  the StoreKitTabHelper object.
- ITunesLinksObserver is converted to use WebState to access
  the StoreKitTabHelper.

PTAL. Thanks!

[sdefresne, 2017-03-06 19:44:04]

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/BUILD.gn (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/BUILD.gn:13: "//ios/chrome/browser",
I think you can remove this deps (//ios/chrome/browser).

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/BUILD.gn:29: "//ios/chrome/browser",
I think you can remove this deps (//ios/chrome/browser).

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/itunes_links_observer.mm (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/itunes_links_observer.mm:12: #include
"ios/web/public/web_state/web_state.h"
#import (this forward-declares UIView, so it is an Objective-C++ file).

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/nat...
File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h
(right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/nat...
ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h:27: //
Designated initializer.
This comment is redundant with "NS_DESIGNATED_INITIALIZER". Please remove it.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper.h (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper.h:16: explicit
StoreKitTabHelper(web::WebState* web_state){};
Remove trailing semi-colon. You can also use = default here (for consistency
with the destructor).

BTW, I prefer when the constructor/destructor are in the implementation file if
it exists (this avoid potentially inlining it in every file that happens to use
this class). This is particularly important for C++ classes that have pointer to
Objective-C object (as the generated inlined constructor/destructor will have
different semantics if the file is included from a .mm file compiled with or
without ARC).

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper.h:25: id<StoreKitLauncher>
store_kit_launcher_;  // weak
This is incorrect. The implementation file is compiled with ARC, so a raw
pointer to an Objective-C object is an owning pointer. If you want to make this
a non-owning pointer (a.k.a. weak pointer), you need to use "__weak".

However, __weak is unsupported if the header file is included in other file
compiled with ARC disabled, so until all our code uses ARC, the recommended way
is to use base::WeakNSProtocol<id<StoreKitLauncher>>. An alternative would be to
mark this as __unsafe_unretained (but as the name implies, this is unsafe).

Also, as base::WeakNSProtocol<> is self-documenting (as is __weak annotation),
you can remove the "// weak" comment.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm:20:
StoreKitTabHelperTest(){};
Remove trailing semi-colon. You can also use = default here (for consistency
with destructor).

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm:26:
StoreKitTabHelper::CreateForWebState(&web_state_);
nit: if possible avoid using SetUp/TearDown in tests and prefer putting the
initialisation/cleanup in the constructor/destructor (this is because gtest
instantiate a new test object for each test, so it is safer to put the code in
constructor/destructor because you cannot forget to call the super-class
implementation while it is quite easy to do with SetUp/TearDown).

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm:38:
EXPECT_TRUE(tab_helper);
You should use ASSERT_TRUE here as it will stop the test if this is false. Then
you do not have to check whether tab_helper is non-null below:

  StoreKitTabHelper* tab_helper = StoreKitTabHelper::FromWebState(&web_state_);
  ASSERT_TRUE(tab_helper);

  id mock_store_kit_launcher =
      [OCMockObject niceMockForProtocol:@protocol(StoreKitLauncher)];
  ...

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1456: id<StoreKitLauncher> storeKitLauncher =
The tab helper may be null during tests, can you check for this?

  id<StoreKitLauncher> storeKitLauncher = nil;
  StoreKitTabHelper* tabHelper =
      StoreKitTabHelper::FromWebState(self.webState);
  if (tabHelper)
    storeKitLauncher = tabHelper->GetLauncher();

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1564:
StoreKitTabHelper::FromWebState(self.webState)->OpenAppStore(appId);
The tab helper may be null during tests, can you check for this?

  StoreKitTabHelper* tabHelper =
      StoreKitTabHelper::FromWebState(self.webState);
  if (tabHelper)
    tabHelper->OpenAppStore(appId);

[pkl (ping after 24h if needed), 2017-03-07 01:17:54]

sdefresne: Thanks! I've made the changes as you've suggested. I also noticed
that requestContextGetter param in the NativeAppNavigationController initializer
is actually redundant now that we have WebState, so I removed it. Let me know if
you disagree or not.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/BUILD.gn (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/BUILD.gn:13: "//ios/chrome/browser",
On 2017/03/06 19:44:03, sdefresne wrote:
> I think you can remove this deps (//ios/chrome/browser).

Done.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/BUILD.gn:29: "//ios/chrome/browser",
On 2017/03/06 19:44:03, sdefresne wrote:
> I think you can remove this deps (//ios/chrome/browser).

Done.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/itunes_links_observer.mm (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/itunes_links_observer.mm:12: #include
"ios/web/public/web_state/web_state.h"
On 2017/03/06 19:44:03, sdefresne wrote:
> #import (this forward-declares UIView, so it is an Objective-C++ file).

Done.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/nat...
File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h
(right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/nat...
ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h:27: //
Designated initializer.
On 2017/03/06 19:44:03, sdefresne wrote:
> This comment is redundant with "NS_DESIGNATED_INITIALIZER". Please remove it.

Done.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper.h (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper.h:16: explicit
StoreKitTabHelper(web::WebState* web_state){};
On 2017/03/06 19:44:03, sdefresne wrote:
> Remove trailing semi-colon. You can also use = default here (for consistency
> with the destructor).
> 
> BTW, I prefer when the constructor/destructor are in the implementation file
if
> it exists (this avoid potentially inlining it in every file that happens to
use
> this class). This is particularly important for C++ classes that have pointer
to
> Objective-C object (as the generated inlined constructor/destructor will have
> different semantics if the file is included from a .mm file compiled with or
> without ARC).

Moved implementation to .mm file.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper.h:25: id<StoreKitLauncher>
store_kit_launcher_;  // weak
On 2017/03/06 19:44:03, sdefresne wrote:
> This is incorrect. The implementation file is compiled with ARC, so a raw
> pointer to an Objective-C object is an owning pointer. If you want to make
this
> a non-owning pointer (a.k.a. weak pointer), you need to use "__weak".
> 
> However, __weak is unsupported if the header file is included in other file
> compiled with ARC disabled, so until all our code uses ARC, the recommended
way
> is to use base::WeakNSProtocol<id<StoreKitLauncher>>. An alternative would be
to
> mark this as __unsafe_unretained (but as the name implies, this is unsafe).
> 
> Also, as base::WeakNSProtocol<> is self-documenting (as is __weak annotation),
> you can remove the "// weak" comment.

Thanks for the explanation. Using base::WeakNSProtocol instead.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm:20:
StoreKitTabHelperTest(){};
On 2017/03/06 19:44:03, sdefresne wrote:
> Remove trailing semi-colon. You can also use = default here (for consistency
> with destructor).

Done.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm:26:
StoreKitTabHelper::CreateForWebState(&web_state_);
On 2017/03/06 19:44:03, sdefresne wrote:
> nit: if possible avoid using SetUp/TearDown in tests and prefer putting the
> initialisation/cleanup in the constructor/destructor (this is because gtest
> instantiate a new test object for each test, so it is safer to put the code in
> constructor/destructor because you cannot forget to call the super-class
> implementation while it is quite easy to do with SetUp/TearDown).
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

Done.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1456: id<StoreKitLauncher> storeKitLauncher =
On 2017/03/06 19:44:03, sdefresne wrote:
> The tab helper may be null during tests, can you check for this?
> 
>   id<StoreKitLauncher> storeKitLauncher = nil;
>   StoreKitTabHelper* tabHelper =
>       StoreKitTabHelper::FromWebState(self.webState);
>   if (tabHelper)
>     storeKitLauncher = tabHelper->GetLauncher();

Done.

https://codereview.chromium.org/2731553005/diff/110001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1564:
StoreKitTabHelper::FromWebState(self.webState)->OpenAppStore(appId);
On 2017/03/06 19:44:03, sdefresne wrote:
> The tab helper may be null during tests, can you check for this?
> 
>   StoreKitTabHelper* tabHelper =
>       StoreKitTabHelper::FromWebState(self.webState);
>   if (tabHelper)
>     tabHelper->OpenAppStore(appId);

Done.

[sdefresne, 2017-03-07 01:43:26]

lgtm

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper.mm (right):

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper.mm:15:
StoreKitTabHelper::~StoreKitTabHelper() = default;
nit: Can you use "{}" here or "= default;" for the constructor (for
consistency)?

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm:15: namespace {
nit, optional: there is no need to put test code in an anonymous namespace (I've
seen discussion on chromium-dev that recommended against it)

[pkl (ping after 24h if needed), 2017-03-07 02:10:02]

Thank you. Address 2nd round of comments.

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper.mm (right):

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper.mm:15:
StoreKitTabHelper::~StoreKitTabHelper() = default;
On 2017/03/07 01:43:26, sdefresne wrote:
> nit: Can you use "{}" here or "= default;" for the constructor (for
> consistency)?

Done. {} everywhere.

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
File ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2731553005/diff/170001/ios/chrome/browser/sto...
ios/chrome/browser/store_kit/store_kit_tab_helper_unittest.mm:15: namespace {
On 2017/03/07 01:43:26, sdefresne wrote:
> nit, optional: there is no need to put test code in an anonymous namespace
(I've
> seen discussion on chromium-dev that recommended against it)

Done, removed anonymous namespace.

[pkl (ping after 24h if needed), 2017-03-07 02:10:11]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-03-07 02:10:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2731553005/#ps190001
(title: "rebased and address sdefresne's 2nd round comments")

[commit-bot: I haz the power, 2017-03-07 02:10:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 05:35:46]

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

[commit-bot: I haz the power, 2017-03-07 05:35:47]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rohitrao (ping after 24h), 2017-03-07 13:38:08]

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/BUI...
File ios/chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/BUI...
ios/chrome/browser/BUILD.gn:103: "//ios/chrome/browser/store_kit",
Does this target actually need to depend on store_kit?

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/itunes_links_observer.mm (right):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/itunes_links_observer.mm:71:
tabHelper->SetLauncher(storeKitLauncher);
Who sets a launcher on this object?  It's a little weird for that call to modify
the underlying StoreKitTabHelper.  Is this meant only for testing?

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (left):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1215: self.storeKitLauncher = nil;
Should we call SetLauncher(nil) here?

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:886: - (id<StoreKitLauncher>)storeKitLauncher {
I'd like to get these methods out of Tab as well, in a followup CL.  Callers can
go through StoreKitTabHelper::FromWebState() directly.

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:888: return tabHelper ? tabHelper->GetLauncher()
: nil;
We call CreateFromWebState() in Tab's init, so there should be no need to test
against nil in either of these methods?

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1565: - (void)openAppStore:(NSString*)appId {
This is another method that should eventually be removed from Tab.

[sdefresne, 2017-03-07 17:11:17]

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:888: return tabHelper ? tabHelper->GetLauncher()
: nil;
On 2017/03/07 13:38:07, rohitrao wrote:
> We call CreateFromWebState() in Tab's init, so there should be no need to test
> against nil in either of these methods?

We can create Tab without TabHelper (see
-initWithWebState:model:attachTabHelpers: that can receive NO for
attachTabHelpers during testing).

[pkl (ping after 24h if needed), 2017-03-07 21:24:46]

Removed more unnecessary code.
sdefresne, rohitrao: Do you want to take another look before I submit?

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/BUI...
File ios/chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/BUI...
ios/chrome/browser/BUILD.gn:103: "//ios/chrome/browser/store_kit",
On 2017/03/07 13:38:07, rohitrao wrote:
> Does this target actually need to depend on store_kit?

Doesn't seem necessary. Removed. Thanks!

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/itunes_links_observer.mm (right):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/itunes_links_observer.mm:71:
tabHelper->SetLauncher(storeKitLauncher);
On 2017/03/07 13:38:07, rohitrao wrote:
> Who sets a launcher on this object?  It's a little weird for that call to
modify
> the underlying StoreKitTabHelper.  Is this meant only for testing?

This ITunesLinksObserver class is not even used. I'm refactoring unused code, so
can't really tell what the intent is. I'm replicating original logic only.

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (left):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1215: self.storeKitLauncher = nil;
On 2017/03/07 13:38:07, rohitrao wrote:
> Should we call SetLauncher(nil) here?

Already removed.

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:886: - (id<StoreKitLauncher>)storeKitLauncher {
On 2017/03/07 13:38:07, rohitrao wrote:
> I'd like to get these methods out of Tab as well, in a followup CL.  Callers
can
> go through StoreKitTabHelper::FromWebState() directly.

Yes, good point. Done.

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:888: return tabHelper ? tabHelper->GetLauncher()
: nil;
On 2017/03/07 17:11:16, sdefresne wrote:
> On 2017/03/07 13:38:07, rohitrao wrote:
> > We call CreateFromWebState() in Tab's init, so there should be no need to
test
> > against nil in either of these methods?
> 
> We can create Tab without TabHelper (see
> -initWithWebState:model:attachTabHelpers: that can receive NO for
> attachTabHelpers during testing).

Removing this...the case is moot.

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:888: return tabHelper ? tabHelper->GetLauncher()
: nil;
On 2017/03/07 13:38:07, rohitrao wrote:
> We call CreateFromWebState() in Tab's init, so there should be no need to test
> against nil in either of these methods?

Removing this...the case is moot.

https://codereview.chromium.org/2731553005/diff/190001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1565: - (void)openAppStore:(NSString*)appId {
On 2017/03/07 13:38:08, rohitrao wrote:
> This is another method that should eventually be removed from Tab.

Yes, removed.

[sdefresne, 2017-03-08 02:53:14]

lgtm

https://codereview.chromium.org/2731553005/diff/230001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/itunes_links_observer.mm (right):

https://codereview.chromium.org/2731553005/diff/230001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/itunes_links_observer.mm:39: new
web::WebStateObserverBridge(webState, self));
optional: can you change this to use base::MakeUnique<>?

  _webStateObserverBridge = base::MakeUnique<web::WebStateObserverBridge>(
      webState, self);

[pkl (ping after 24h if needed), 2017-03-08 14:27:41]

https://codereview.chromium.org/2731553005/diff/230001/ios/chrome/browser/itu...
File ios/chrome/browser/itunes_links/itunes_links_observer.mm (right):

https://codereview.chromium.org/2731553005/diff/230001/ios/chrome/browser/itu...
ios/chrome/browser/itunes_links/itunes_links_observer.mm:39: new
web::WebStateObserverBridge(webState, self));
On 2017/03/08 02:53:14, sdefresne wrote:
> optional: can you change this to use base::MakeUnique<>?
> 
>   _webStateObserverBridge = base::MakeUnique<web::WebStateObserverBridge>(
>       webState, self);

I'll do a separate CL as this is unrelated to this.

[pkl (ping after 24h if needed), 2017-03-08 14:27:48]

The CQ bit was checked by pkl@chromium.org

[commit-bot: I haz the power, 2017-03-08 14:27:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 15:56:57]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1488983268593940,
"parent_rev": "8638ef810c10cee09e467b2ecd678341e12fc892", "commit_rev":
"d6e73e57a224c74a169abbcd8f2507fd1a5daa7b"}

[commit-bot: I haz the power, 2017-03-08 15:57:38]

Description was changed from

==========
Introduced StoreKitTabHelper class

Removed _tab from NativeAppNavigationController as everything can
be obtained from WebState.

BUG=684063
==========

to

==========
Introduced StoreKitTabHelper class

Removed _tab from NativeAppNavigationController as everything can
be obtained from WebState.

BUG=684063

Review-Url: https://codereview.chromium.org/2731553005
Cr-Commit-Position: refs/heads/master@{#455460}
Committed:
https://chromium.googlesource.com/chromium/src/+/d6e73e57a224c74a169abbcd8f25...
==========

[commit-bot: I haz the power, 2017-03-08 15:57:44]

Committed patchset #13 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/d6e73e57a224c74a169abbcd8f25...