Issue 2487623002: Notify Files App when ARC++ app is installed/removed

oka

Description was changed from ========== Remove unused variable tasks_ from task_controller. BUG=None TEST=manually tested. ========== ...

4 years, 1 month ago (2016-11-08 07:22:41 UTC) #1

oka

Description was changed from ========== Remove unused variable tasks_ from task_controller. BUG=None TEST=manually tested. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ...

4 years, 1 month ago (2016-11-16 08:49:29 UTC) #2

oka

oka@chromium.org changed reviewers: + fukino@chromium.org, kinaba@chromium.org

4 years, 1 month ago (2016-11-16 08:49:29 UTC) #3

kinaba

kinaba@chromium.org changed reviewers: + hidehiko@chromium.org, yusukes@chromium.org

4 years, 1 month ago (2016-11-17 01:47:19 UTC) #5

kinaba

+hidehiko as arc/ OWNER and +yusukes for ArcIntentHelper author. (Sorry, my review may be slow ...

4 years, 1 month ago (2016-11-17 01:47:19 UTC) #6

Yusuke Sato

initial comment. more to come. https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode40 chrome/browser/chromeos/extensions/file_manager/event_router.cc:40: #include "components/arc/intent_helper/arc_intent_helper_bridge.h" nit: Remove. ...

4 years, 1 month ago (2016-11-17 02:29:55 UTC) #7

Yusuke Sato

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode506 chrome/browser/chromeos/extensions/file_manager/event_router.cc:506: DCHECK(bridge); On 2016/11/17 02:29:54, Yusuke Sato wrote: > On ...

4 years, 1 month ago (2016-11-17 04:22:38 UTC) #8

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:506:
DCHECK(bridge);
On 2016/11/17 02:29:54, Yusuke Sato wrote:
> On 2016/11/17 01:47:19, kinaba wrote:
> > When ARC is not enabled (either by config or when the device does not
support
> it
> > at all),
> > |bridge| may be null, IIUC.
> > 
> 
> Yeah, and you might have to retry later to avoid introducing a race. I'll
> comment more on this later.

Sorry I misread your code. ArcIntentHelperBridge instance is created in
ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() regardless of whether
ARC is supported or not, so you code doesn't seem to have a race.

hidehiko:
Can |bridge| be nullptr here? I don't remember if ArcServiceManager's Shutdown()
is actually called on production code.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid
to assume the class is singleton.
Adding more global pointers to components/arc/ seems sad. We already have
enough. Instead of this, what about the following:

* Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
* Add [set_]arc_intent_helper_bridge() setter/getter to ArcServiceManager.
* Add arc_intent_helper_bridge_ raw pointer (member variable) to
ArcServiceManager;
* Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
ArcIntentHelperBridge's ctor, and
ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.
* In chrome/browser/, get an ArcIntentHelperBridge instance with
ArcServiceManager::Get()->arc_intent_helper_bridge().

ArcServiceManager is already a singleton and has Get() method. The manager also
has getters like icon_loader() for allowing chrome/browser/ code to use
components/arc/'s objects. The proposal above follows the current way to some
extent.

hidehiko

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode506 chrome/browser/chromeos/extensions/file_manager/event_router.cc:506: DCHECK(bridge); On 2016/11/17 04:22:37, Yusuke Sato wrote: > On ...

4 years, 1 month ago (2016-11-17 07:34:19 UTC) #9

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:506:
DCHECK(bridge);
On 2016/11/17 04:22:37, Yusuke Sato wrote:
> On 2016/11/17 02:29:54, Yusuke Sato wrote:
> > On 2016/11/17 01:47:19, kinaba wrote:
> > > When ARC is not enabled (either by config or when the device does not
> support
> > it
> > > at all),
> > > |bridge| may be null, IIUC.
> > > 
> > 
> > Yeah, and you might have to retry later to avoid introducing a race. I'll
> > comment more on this later.
> 
> Sorry I misread your code. ArcIntentHelperBridge instance is created in
> ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() regardless of whether
> ARC is supported or not, so you code doesn't seem to have a race.
> 
> hidehiko:
> Can |bridge| be nullptr here? I don't remember if ArcServiceManager's
Shutdown()
> is actually called on production code.

> Can |bridge| be nullptr here?

I think no. In past, the launcher creation was guarded by a flag IIRC, but
nowadays it is created always. I'm not sure it is ensured in all test cases,
though.

> I don't remember if ArcServiceManager's Shutdown()
> is actually called on production code.

It is.
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...

Note that the Shutdown is called before the BrowserContextKeyedService is
destructed.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid
to assume the class is singleton.
On 2016/11/17 04:22:37, Yusuke Sato wrote:
> Adding more global pointers to components/arc/ seems sad. We already have
> enough. Instead of this, what about the following:
> 
> * Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
> * Add [set_]arc_intent_helper_bridge() setter/getter to ArcServiceManager.
> * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> ArcServiceManager;
> * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> ArcIntentHelperBridge's ctor, and
> ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.
> * In chrome/browser/, get an ArcIntentHelperBridge instance with
> ArcServiceManager::Get()->arc_intent_helper_bridge().
> 
> ArcServiceManager is already a singleton and has Get() method. The manager
also
> has getters like icon_loader() for allowing chrome/browser/ code to use
> components/arc/'s objects. The proposal above follows the current way to some
> extent.

> * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
ArcIntentHelperBridge's ctor, and
ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.

I do not want this. If we'll get rid of global ptr, I prefer calling the setter
in ArcServiceLauncher::Initialize() instead.

> ArcServiceManager is already a singleton and has Get() method. The manager
also
> has getters like icon_loader() for allowing chrome/browser/ code to use
> components/arc/'s objects. The proposal above follows the current way to some
> extent.

We have some getters, but those are not for ArcService classes, so this looks
something new to me, too. (And that's why I recommended to oka@ san to use
global ptr.)

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:52: virtual void
OnAppsUpdated() = 0;
Document please.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:61: static
ArcIntentHelperBridge* Get();
It's better to note this is something workaround, and encouraged to avoid as
much as possible.
Also, could you document when this returns an instance, and when not?

Yusuke Sato

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode506 chrome/browser/chromeos/extensions/file_manager/event_router.cc:506: DCHECK(bridge); On 2016/11/17 07:34:19, hidehiko wrote: > On 2016/11/17 ...

4 years, 1 month ago (2016-11-17 09:15:27 UTC) #10

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:506:
DCHECK(bridge);
On 2016/11/17 07:34:19, hidehiko wrote:
> On 2016/11/17 04:22:37, Yusuke Sato wrote:
> > On 2016/11/17 02:29:54, Yusuke Sato wrote:
> > > On 2016/11/17 01:47:19, kinaba wrote:
> > > > When ARC is not enabled (either by config or when the device does not
> > support
> > > it
> > > > at all),
> > > > |bridge| may be null, IIUC.
> > > > 
> > > 
> > > Yeah, and you might have to retry later to avoid introducing a race. I'll
> > > comment more on this later.
> > 
> > Sorry I misread your code. ArcIntentHelperBridge instance is created in
> > ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() regardless of
whether
> > ARC is supported or not, so you code doesn't seem to have a race.
> > 
> > hidehiko:
> > Can |bridge| be nullptr here? I don't remember if ArcServiceManager's
> Shutdown()
> > is actually called on production code.
> 
> > Can |bridge| be nullptr here?
> 
> I think no. In past, the launcher creation was guarded by a flag IIRC, but
> nowadays it is created always. I'm not sure it is ensured in all test cases,
> though.
> 
> > I don't remember if ArcServiceManager's Shutdown()
> > is actually called on production code.
> 
> It is.
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...
> 
> Note that the Shutdown is called before the BrowserContextKeyedService is
> destructed.

Got it, thanks for refreshing my memory.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid
to assume the class is singleton.
On 2016/11/17 07:34:19, hidehiko wrote:
> On 2016/11/17 04:22:37, Yusuke Sato wrote:
> > Adding more global pointers to components/arc/ seems sad. We already have
> > enough. Instead of this, what about the following:
> > 
> > * Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
> > * Add [set_]arc_intent_helper_bridge() setter/getter to ArcServiceManager.
> > * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> > ArcServiceManager;
> > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> > ArcIntentHelperBridge's ctor, and
> > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.
> > * In chrome/browser/, get an ArcIntentHelperBridge instance with
> > ArcServiceManager::Get()->arc_intent_helper_bridge().
> > 
> > ArcServiceManager is already a singleton and has Get() method. The manager
> also
> > has getters like icon_loader() for allowing chrome/browser/ code to use
> > components/arc/'s objects. The proposal above follows the current way to
some
> > extent.
> 
> 
> > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> ArcIntentHelperBridge's ctor, and
> ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.
> 
> I do not want this. If we'll get rid of global ptr, I prefer calling the
setter
> in ArcServiceLauncher::Initialize() instead.
> 
> > ArcServiceManager is already a singleton and has Get() method. The manager
> also
> > has getters like icon_loader() for allowing chrome/browser/ code to use
> > components/arc/'s objects. The proposal above follows the current way to
some
> > extent.
> 
> We have some getters, but those are not for ArcService classes, so this looks
> something new to me, too. (And that's why I recommended to oka@ san to use
> global ptr.)

Well, I wanted to make the code *look* slightly better (although it does not
resolve anything and the member variable is still effectively like a global
pointer) but if it's not for your taste, that's fine :)

BTW, I have another idea to remove g_arc_intent_helper_bridge (this time,
really) and confirmed that hidehiko@ is okay with it. The idea is:

* Continue to define the Observer class, but probably in a dedicated file in
components/arc/intent_helper/.
* Add AddObserver and RemoveObserver to ArcIntentHelperBridge.
* Let ArcServiceManager observe ArcIntentHelperBridge. You can call
ArcIntentHelperBridge::AddObserver(arc_service_manager); in
ArcServiceLauncher::Initialize() where ArcIntentHelperBridge is created and
added to ArcServiceManager.
* Since ArcServiceManager always outlives the ArcIntentHelperBridge object,
ArcServiceManager doesn't have to call
ArcIntentHelperBridge::RemoveObserver(this); (we need some code comments
though.)
* Let your EventRouter code observe ArcServiceManager, instead of
ArcIntentHelperBridge. You can call ArcServiceManager::AddObserver() in
EventRouter::ObserveEvents().
* You can also call ArcServiceManager::RemoveObserver() in
EventRouter::Shutdown(). When EventRouter::Shutdown() is called,
ArcIntentHelperBridge has already been gone, but ArcServiceManager is still
there.
* Obviously, you also have to chain the OnAppsUpdated() calls.
ArcIntentHelperBridge calls its observers' OnAppsUpdated(), which is ASM's
OnAppsUpdated(). ASM's OnAppsUpdated() calls its observers' OnAppsUpdated(),
which is EventRouter::OnAppsUpdated().

WDYT?

oka

The CQ bit was checked by oka@chromium.org to run a CQ dry run

4 years, 1 month ago (2016-11-18 08:19:56 UTC) #11

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/60001

4 years, 1 month ago (2016-11-18 08:20:17 UTC) #12

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 08:35:19 UTC) #13

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-generic_chromium_compile_only_ng/builds/237682) linux_chromium_chromeos_ozone_rel_ng on ...

4 years, 1 month ago (2016-11-18 08:35:20 UTC) #14

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years, 1 month ago (2016-11-21 06:03:06 UTC) #15

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years, 1 month ago (2016-11-21 06:03:40 UTC) #16

oka

The CQ bit was checked by oka@chromium.org to run a CQ dry run

4 years, 1 month ago (2016-11-21 06:35:55 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/100001

4 years, 1 month ago (2016-11-21 06:36:06 UTC) #18

oka

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode40 chrome/browser/chromeos/extensions/file_manager/event_router.cc:40: #include "components/arc/intent_helper/arc_intent_helper_bridge.h" On 2016/11/17 02:29:54, Yusuke Sato wrote: > ...

4 years, 1 month ago (2016-11-21 07:02:15 UTC) #19

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:40: #include
"components/arc/intent_helper/arc_intent_helper_bridge.h"
On 2016/11/17 02:29:54, Yusuke Sato wrote:
> nit: Remove. You can rely on the header file in this case.
> 
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
> reads "However, any includes present in the related header do not need to be
> included again in the related cc (i.e., foo.cc can rely on foo.h's includes)."

Done.

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:507:
bridge->AddObserver(this);
On 2016/11/17 02:29:54, Yusuke Sato wrote:
> AddObserver should almost always be paired with RemoveObserver. For other
> observer interfaces, EventRouter::Shutdown() seems to call RemoveObserver in
the
> reverse order. Shouldn't we do that too?

Done.

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.h (right):

https://codereview.chromium.org/2487623002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.h:74: void
OnAppsUpdated() override;
On 2016/11/17 02:29:54, Yusuke Sato wrote:
> // arc::ArcIntentHelperBridge::Observer oderrides:
> 
> to be consistent with L76.

Done.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid
to assume the class is singleton.
On 2016/11/17 04:22:37, Yusuke Sato wrote:
> Adding more global pointers to components/arc/ seems sad. We already have
> enough. Instead of this, what about the following:
> 
> * Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
> * Add [set_]arc_intent_helper_bridge() setter/getter to ArcServiceManager.
> * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> ArcServiceManager;
> * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> ArcIntentHelperBridge's ctor, and
> ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.
> * In chrome/browser/, get an ArcIntentHelperBridge instance with
> ArcServiceManager::Get()->arc_intent_helper_bridge().
> 
> ArcServiceManager is already a singleton and has Get() method. The manager
also
> has getters like icon_loader() for allowing chrome/browser/ code to use
> components/arc/'s objects. The proposal above follows the current way to some
> extent.

Abe-san suggested this solution. @hidehiko what do you think?

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid
to assume the class is singleton.
On 2016/11/17 09:15:27, Yusuke Sato wrote:
> On 2016/11/17 07:34:19, hidehiko wrote:
> > On 2016/11/17 04:22:37, Yusuke Sato wrote:
> > > Adding more global pointers to components/arc/ seems sad. We already have
> > > enough. Instead of this, what about the following:
> > > 
> > > * Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
> > > * Add [set_]arc_intent_helper_bridge() setter/getter to ArcServiceManager.
> > > * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> > > ArcServiceManager;
> > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> > > ArcIntentHelperBridge's ctor, and
> > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its
dtor.
> > > * In chrome/browser/, get an ArcIntentHelperBridge instance with
> > > ArcServiceManager::Get()->arc_intent_helper_bridge().
> > > 
> > > ArcServiceManager is already a singleton and has Get() method. The manager
> > also
> > > has getters like icon_loader() for allowing chrome/browser/ code to use
> > > components/arc/'s objects. The proposal above follows the current way to
> some
> > > extent.
> > 
> > 
> > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> > ArcIntentHelperBridge's ctor, and
> > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its dtor.
> > 
> > I do not want this. If we'll get rid of global ptr, I prefer calling the
> setter
> > in ArcServiceLauncher::Initialize() instead.
> > 
> > > ArcServiceManager is already a singleton and has Get() method. The manager
> > also
> > > has getters like icon_loader() for allowing chrome/browser/ code to use
> > > components/arc/'s objects. The proposal above follows the current way to
> some
> > > extent.
> > 
> > We have some getters, but those are not for ArcService classes, so this
looks
> > something new to me, too. (And that's why I recommended to oka@ san to use
> > global ptr.)
> 
> Well, I wanted to make the code *look* slightly better (although it does not
> resolve anything and the member variable is still effectively like a global
> pointer) but if it's not for your taste, that's fine :)
> 
> BTW, I have another idea to remove g_arc_intent_helper_bridge (this time,
> really) and confirmed that hidehiko@ is okay with it. The idea is:
> 
> * Continue to define the Observer class, but probably in a dedicated file in
> components/arc/intent_helper/.
> * Add AddObserver and RemoveObserver to ArcIntentHelperBridge.
> * Let ArcServiceManager observe ArcIntentHelperBridge. You can call
> ArcIntentHelperBridge::AddObserver(arc_service_manager); in
> ArcServiceLauncher::Initialize() where ArcIntentHelperBridge is created and
> added to ArcServiceManager.
> * Since ArcServiceManager always outlives the ArcIntentHelperBridge object,
> ArcServiceManager doesn't have to call
> ArcIntentHelperBridge::RemoveObserver(this); (we need some code comments
> though.)
> * Let your EventRouter code observe ArcServiceManager, instead of
> ArcIntentHelperBridge. You can call ArcServiceManager::AddObserver() in
> EventRouter::ObserveEvents().
> * You can also call ArcServiceManager::RemoveObserver() in
> EventRouter::Shutdown(). When EventRouter::Shutdown() is called,
> ArcIntentHelperBridge has already been gone, but ArcServiceManager is still
> there.
> * Obviously, you also have to chain the OnAppsUpdated() calls.
> ArcIntentHelperBridge calls its observers' OnAppsUpdated(), which is ASM's
> OnAppsUpdated(). ASM's OnAppsUpdated() calls its observers' OnAppsUpdated(),
> which is EventRouter::OnAppsUpdated().
> 
> WDYT?
>  
> 
> 
> 
> 

Thank you for the suggestion. I will try the construct in the next patch.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:192: for (auto&
observer : observer_list_) {
On 2016/11/17 02:29:54, Yusuke Sato wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:52: virtual void
OnAppsUpdated() = 0;
On 2016/11/17 07:34:19, hidehiko wrote:
> Document please.

Done.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:53: };
On 2016/11/17 02:29:54, Yusuke Sato wrote:
> Don't we need
> 
>  protected:
>   virtual ~Observer() {}
> 
> ?

Done.

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:61: static
ArcIntentHelperBridge* Get();
On 2016/11/17 07:34:19, hidehiko wrote:
> It's better to note this is something workaround, and encouraged to avoid as
> much as possible.
> Also, could you document when this returns an instance, and when not?

Done.

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years, 1 month ago (2016-11-21 07:18:42 UTC) #20

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years, 1 month ago (2016-11-21 07:18:56 UTC) #21

oka

The CQ bit was checked by oka@chromium.org to run a CQ dry run

4 years, 1 month ago (2016-11-21 07:50:59 UTC) #22

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/160001

4 years, 1 month ago (2016-11-21 07:51:04 UTC) #23

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-21 10:16:20 UTC) #24

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/276691)

4 years, 1 month ago (2016-11-21 10:16:21 UTC) #25

Yusuke Sato

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_helper/arc_intent_helper_bridge.cc File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_helper/arc_intent_helper_bridge.cc#newcode26 components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid to assume the class is singleton. ...

4 years, 1 month ago (2016-11-21 17:21:05 UTC) #26

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid
to assume the class is singleton.
On 2016/11/21 07:02:15, oka wrote:
> On 2016/11/17 09:15:27, Yusuke Sato wrote:
> > On 2016/11/17 07:34:19, hidehiko wrote:
> > > On 2016/11/17 04:22:37, Yusuke Sato wrote:
> > > > Adding more global pointers to components/arc/ seems sad. We already
have
> > > > enough. Instead of this, what about the following:
> > > > 
> > > > * Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
> > > > * Add [set_]arc_intent_helper_bridge() setter/getter to
ArcServiceManager.
> > > > * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> > > > ArcServiceManager;
> > > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> > > > ArcIntentHelperBridge's ctor, and
> > > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its
> dtor.
> > > > * In chrome/browser/, get an ArcIntentHelperBridge instance with
> > > > ArcServiceManager::Get()->arc_intent_helper_bridge().
> > > > 
> > > > ArcServiceManager is already a singleton and has Get() method. The
manager
> > > also
> > > > has getters like icon_loader() for allowing chrome/browser/ code to use
> > > > components/arc/'s objects. The proposal above follows the current way to
> > some
> > > > extent.
> > > 
> > > 
> > > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> > > ArcIntentHelperBridge's ctor, and
> > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its
dtor.
> > > 
> > > I do not want this. If we'll get rid of global ptr, I prefer calling the
> > setter
> > > in ArcServiceLauncher::Initialize() instead.
> > > 
> > > > ArcServiceManager is already a singleton and has Get() method. The
manager
> > > also
> > > > has getters like icon_loader() for allowing chrome/browser/ code to use
> > > > components/arc/'s objects. The proposal above follows the current way to
> > some
> > > > extent.
> > > 
> > > We have some getters, but those are not for ArcService classes, so this
> looks
> > > something new to me, too. (And that's why I recommended to oka@ san to use
> > > global ptr.)
> > 
> > Well, I wanted to make the code *look* slightly better (although it does not
> > resolve anything and the member variable is still effectively like a global
> > pointer) but if it's not for your taste, that's fine :)
> > 
> > BTW, I have another idea to remove g_arc_intent_helper_bridge (this time,
> > really) and confirmed that hidehiko@ is okay with it. The idea is:
> > 
> > * Continue to define the Observer class, but probably in a dedicated file in
> > components/arc/intent_helper/.
> > * Add AddObserver and RemoveObserver to ArcIntentHelperBridge.
> > * Let ArcServiceManager observe ArcIntentHelperBridge. You can call
> > ArcIntentHelperBridge::AddObserver(arc_service_manager); in
> > ArcServiceLauncher::Initialize() where ArcIntentHelperBridge is created and
> > added to ArcServiceManager.
> > * Since ArcServiceManager always outlives the ArcIntentHelperBridge object,
> > ArcServiceManager doesn't have to call
> > ArcIntentHelperBridge::RemoveObserver(this); (we need some code comments
> > though.)
> > * Let your EventRouter code observe ArcServiceManager, instead of
> > ArcIntentHelperBridge. You can call ArcServiceManager::AddObserver() in
> > EventRouter::ObserveEvents().
> > * You can also call ArcServiceManager::RemoveObserver() in
> > EventRouter::Shutdown(). When EventRouter::Shutdown() is called,
> > ArcIntentHelperBridge has already been gone, but ArcServiceManager is still
> > there.
> > * Obviously, you also have to chain the OnAppsUpdated() calls.
> > ArcIntentHelperBridge calls its observers' OnAppsUpdated(), which is ASM's
> > OnAppsUpdated(). ASM's OnAppsUpdated() calls its observers' OnAppsUpdated(),
> > which is EventRouter::OnAppsUpdated().
> > 
> > WDYT?
> >  
> > 
> > 
> > 
> > 
> 
> Thank you for the suggestion. I will try the construct in the next patch.

What do you mean by "the next patch"? Next patch set in this CL, or different
CL? Since this is not a small TODO but more like a structural change, I'd prefer
the former unless this is urgent.

https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:416:
bridge->RemoveObserver(this);
How did you test this? |bridge| might be NULL.

https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
components/arc/intent_helper/arc_intent_helper_bridge.h:52: // OnAppsUpdated is
called when intent helper is updated.
// Called when app's intent filter is updated.

?

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years ago (2016-11-24 02:41:36 UTC) #27

oka

Removed global pointer in arc_intent_helper_bridge and separeted observer class out.

4 years ago (2016-11-24 13:32:00 UTC) #29

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/240001

4 years ago (2016-11-24 15:12:56 UTC) #31

oka

PTAL https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_helper/arc_intent_helper_bridge.cc File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_helper/arc_intent_helper_bridge.cc#newcode26 components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid to assume the class is ...

4 years ago (2016-11-24 15:14:18 UTC) #32

PTAL

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka): Avoid
to assume the class is singleton.
On 2016/11/21 17:21:05, Yusuke Sato wrote:
> On 2016/11/21 07:02:15, oka wrote:
> > On 2016/11/17 09:15:27, Yusuke Sato wrote:
> > > On 2016/11/17 07:34:19, hidehiko wrote:
> > > > On 2016/11/17 04:22:37, Yusuke Sato wrote:
> > > > > Adding more global pointers to components/arc/ seems sad. We already
> have
> > > > > enough. Instead of this, what about the following:
> > > > > 
> > > > > * Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
> > > > > * Add [set_]arc_intent_helper_bridge() setter/getter to
> ArcServiceManager.
> > > > > * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> > > > > ArcServiceManager;
> > > > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> > > > > ArcIntentHelperBridge's ctor, and
> > > > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its
> > dtor.
> > > > > * In chrome/browser/, get an ArcIntentHelperBridge instance with
> > > > > ArcServiceManager::Get()->arc_intent_helper_bridge().
> > > > > 
> > > > > ArcServiceManager is already a singleton and has Get() method. The
> manager
> > > > also
> > > > > has getters like icon_loader() for allowing chrome/browser/ code to
use
> > > > > components/arc/'s objects. The proposal above follows the current way
to
> > > some
> > > > > extent.
> > > > 
> > > > 
> > > > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this) in
> > > > ArcIntentHelperBridge's ctor, and
> > > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its
> dtor.
> > > > 
> > > > I do not want this. If we'll get rid of global ptr, I prefer calling the
> > > setter
> > > > in ArcServiceLauncher::Initialize() instead.
> > > > 
> > > > > ArcServiceManager is already a singleton and has Get() method. The
> manager
> > > > also
> > > > > has getters like icon_loader() for allowing chrome/browser/ code to
use
> > > > > components/arc/'s objects. The proposal above follows the current way
to
> > > some
> > > > > extent.
> > > > 
> > > > We have some getters, but those are not for ArcService classes, so this
> > looks
> > > > something new to me, too. (And that's why I recommended to oka@ san to
use
> > > > global ptr.)
> > > 
> > > Well, I wanted to make the code *look* slightly better (although it does
not
> > > resolve anything and the member variable is still effectively like a
global
> > > pointer) but if it's not for your taste, that's fine :)
> > > 
> > > BTW, I have another idea to remove g_arc_intent_helper_bridge (this time,
> > > really) and confirmed that hidehiko@ is okay with it. The idea is:
> > > 
> > > * Continue to define the Observer class, but probably in a dedicated file
in
> > > components/arc/intent_helper/.
> > > * Add AddObserver and RemoveObserver to ArcIntentHelperBridge.
> > > * Let ArcServiceManager observe ArcIntentHelperBridge. You can call
> > > ArcIntentHelperBridge::AddObserver(arc_service_manager); in
> > > ArcServiceLauncher::Initialize() where ArcIntentHelperBridge is created
and
> > > added to ArcServiceManager.
> > > * Since ArcServiceManager always outlives the ArcIntentHelperBridge
object,
> > > ArcServiceManager doesn't have to call
> > > ArcIntentHelperBridge::RemoveObserver(this); (we need some code comments
> > > though.)
> > > * Let your EventRouter code observe ArcServiceManager, instead of
> > > ArcIntentHelperBridge. You can call ArcServiceManager::AddObserver() in
> > > EventRouter::ObserveEvents().
> > > * You can also call ArcServiceManager::RemoveObserver() in
> > > EventRouter::Shutdown(). When EventRouter::Shutdown() is called,
> > > ArcIntentHelperBridge has already been gone, but ArcServiceManager is
still
> > > there.
> > > * Obviously, you also have to chain the OnAppsUpdated() calls.
> > > ArcIntentHelperBridge calls its observers' OnAppsUpdated(), which is ASM's
> > > OnAppsUpdated(). ASM's OnAppsUpdated() calls its observers'
OnAppsUpdated(),
> > > which is EventRouter::OnAppsUpdated().
> > > 
> > > WDYT?
> > >  
> > > 
> > > 
> > > 
> > > 
> > 
> > Thank you for the suggestion. I will try the construct in the next patch.
> 
> What do you mean by "the next patch"? Next patch set in this CL, or different
> CL? Since this is not a small TODO but more like a structural change, I'd
prefer
> the former unless this is urgent.

Updated the construct as you suggested.

https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:416:
bridge->RemoveObserver(this);
On 2016/11/21 17:21:05, Yusuke Sato wrote:
> How did you test this? |bridge| might be NULL.

Test procedure is written in https://codereview.chromium.org/2513493002/
I copied it in this CL's description.

I updated the construction to chain OnAppsUpdated.

https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
components/arc/intent_helper/arc_intent_helper_bridge.h:52: // OnAppsUpdated is
called when intent helper is updated.
On 2016/11/21 17:21:05, Yusuke Sato wrote:
> // Called when app's intent filter is updated.
> 
> ?

Done.

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years ago (2016-11-24 15:14:52 UTC) #33

oka

On 2016/11/24 15:14:18, oka wrote: > PTAL > > https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_helper/arc_intent_helper_bridge.cc > File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): > ...

4 years ago (2016-11-24 15:15:09 UTC) #34

On 2016/11/24 15:14:18, oka wrote:
> PTAL
> 
>
https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
> File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):
> 
>
https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
> components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka):
Avoid
> to assume the class is singleton.
> On 2016/11/21 17:21:05, Yusuke Sato wrote:
> > On 2016/11/21 07:02:15, oka wrote:
> > > On 2016/11/17 09:15:27, Yusuke Sato wrote:
> > > > On 2016/11/17 07:34:19, hidehiko wrote:
> > > > > On 2016/11/17 04:22:37, Yusuke Sato wrote:
> > > > > > Adding more global pointers to components/arc/ seems sad. We already
> > have
> > > > > > enough. Instead of this, what about the following:
> > > > > > 
> > > > > > * Remove g_arc_intent_helper_bridge and ArcIntentHelperBridge::Get()
> > > > > > * Add [set_]arc_intent_helper_bridge() setter/getter to
> > ArcServiceManager.
> > > > > > * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> > > > > > ArcServiceManager;
> > > > > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this)
in
> > > > > > ArcIntentHelperBridge's ctor, and
> > > > > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in
its
> > > dtor.
> > > > > > * In chrome/browser/, get an ArcIntentHelperBridge instance with
> > > > > > ArcServiceManager::Get()->arc_intent_helper_bridge().
> > > > > > 
> > > > > > ArcServiceManager is already a singleton and has Get() method. The
> > manager
> > > > > also
> > > > > > has getters like icon_loader() for allowing chrome/browser/ code to
> use
> > > > > > components/arc/'s objects. The proposal above follows the current
way
> to
> > > > some
> > > > > > extent.
> > > > > 
> > > > > 
> > > > > > * Call ArcServiceManager::Get()->set_arc_intent_helper_bridge(this)
in
> > > > > ArcIntentHelperBridge's ctor, and
> > > > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in its
> > dtor.
> > > > > 
> > > > > I do not want this. If we'll get rid of global ptr, I prefer calling
the
> > > > setter
> > > > > in ArcServiceLauncher::Initialize() instead.
> > > > > 
> > > > > > ArcServiceManager is already a singleton and has Get() method. The
> > manager
> > > > > also
> > > > > > has getters like icon_loader() for allowing chrome/browser/ code to
> use
> > > > > > components/arc/'s objects. The proposal above follows the current
way
> to
> > > > some
> > > > > > extent.
> > > > > 
> > > > > We have some getters, but those are not for ArcService classes, so
this
> > > looks
> > > > > something new to me, too. (And that's why I recommended to oka@ san to
> use
> > > > > global ptr.)
> > > > 
> > > > Well, I wanted to make the code *look* slightly better (although it does
> not
> > > > resolve anything and the member variable is still effectively like a
> global
> > > > pointer) but if it's not for your taste, that's fine :)
> > > > 
> > > > BTW, I have another idea to remove g_arc_intent_helper_bridge (this
time,
> > > > really) and confirmed that hidehiko@ is okay with it. The idea is:
> > > > 
> > > > * Continue to define the Observer class, but probably in a dedicated
file
> in
> > > > components/arc/intent_helper/.
> > > > * Add AddObserver and RemoveObserver to ArcIntentHelperBridge.
> > > > * Let ArcServiceManager observe ArcIntentHelperBridge. You can call
> > > > ArcIntentHelperBridge::AddObserver(arc_service_manager); in
> > > > ArcServiceLauncher::Initialize() where ArcIntentHelperBridge is created
> and
> > > > added to ArcServiceManager.
> > > > * Since ArcServiceManager always outlives the ArcIntentHelperBridge
> object,
> > > > ArcServiceManager doesn't have to call
> > > > ArcIntentHelperBridge::RemoveObserver(this); (we need some code comments
> > > > though.)
> > > > * Let your EventRouter code observe ArcServiceManager, instead of
> > > > ArcIntentHelperBridge. You can call ArcServiceManager::AddObserver() in
> > > > EventRouter::ObserveEvents().
> > > > * You can also call ArcServiceManager::RemoveObserver() in
> > > > EventRouter::Shutdown(). When EventRouter::Shutdown() is called,
> > > > ArcIntentHelperBridge has already been gone, but ArcServiceManager is
> still
> > > > there.
> > > > * Obviously, you also have to chain the OnAppsUpdated() calls.
> > > > ArcIntentHelperBridge calls its observers' OnAppsUpdated(), which is
ASM's
> > > > OnAppsUpdated(). ASM's OnAppsUpdated() calls its observers'
> OnAppsUpdated(),
> > > > which is EventRouter::OnAppsUpdated().
> > > > 
> > > > WDYT?
> > > >  
> > > > 
> > > > 
> > > > 
> > > > 
> > > 
> > > Thank you for the suggestion. I will try the construct in the next patch.
> > 
> > What do you mean by "the next patch"? Next patch set in this CL, or
different
> > CL? Since this is not a small TODO but more like a structural change, I'd
> prefer
> > the former unless this is urgent.
> 
> Updated the construct as you suggested.
> 
>
https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
> File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):
> 
>
https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
> chrome/browser/chromeos/extensions/file_manager/event_router.cc:416:
> bridge->RemoveObserver(this);
> On 2016/11/21 17:21:05, Yusuke Sato wrote:
> > How did you test this? |bridge| might be NULL.
> 
> Test procedure is written in https://codereview.chromium.org/2513493002/
> I copied it in this CL's description.
> 
> I updated the construction to chain OnAppsUpdated.
> 
>
https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
> File components/arc/intent_helper/arc_intent_helper_bridge.h (right):
> 
>
https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
> components/arc/intent_helper/arc_intent_helper_bridge.h:52: // OnAppsUpdated
is
> called when intent helper is updated.
> On 2016/11/21 17:21:05, Yusuke Sato wrote:
> > // Called when app's intent filter is updated.
> > 
> > ?
> 
> Done.

I will perform the following test tomorrow:
Manually tested using Minnie with this CL 
https://codereview.chromium.org/2513493002/:
1. Select a PDF file on Files App.
2. Install Kindle.
3. Confirm "more action..." appears on the context menu and Open
button's UI changes to be able to open Kindle.

commit-bot: I haz the power

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

4 years ago (2016-11-24 16:09:15 UTC) #35

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years ago (2016-11-25 11:51:52 UTC) #37

oka

On 2016/11/24 15:15:09, oka wrote: > On 2016/11/24 15:14:18, oka wrote: > > PTAL > ...

4 years ago (2016-11-25 11:52:33 UTC) #38

On 2016/11/24 15:15:09, oka wrote:
> On 2016/11/24 15:14:18, oka wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
> > File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):
> > 
> >
>
https://codereview.chromium.org/2487623002/diff/40001/components/arc/intent_h...
> > components/arc/intent_helper/arc_intent_helper_bridge.cc:26: // TODO(oka):
> Avoid
> > to assume the class is singleton.
> > On 2016/11/21 17:21:05, Yusuke Sato wrote:
> > > On 2016/11/21 07:02:15, oka wrote:
> > > > On 2016/11/17 09:15:27, Yusuke Sato wrote:
> > > > > On 2016/11/17 07:34:19, hidehiko wrote:
> > > > > > On 2016/11/17 04:22:37, Yusuke Sato wrote:
> > > > > > > Adding more global pointers to components/arc/ seems sad. We
already
> > > have
> > > > > > > enough. Instead of this, what about the following:
> > > > > > > 
> > > > > > > * Remove g_arc_intent_helper_bridge and
ArcIntentHelperBridge::Get()
> > > > > > > * Add [set_]arc_intent_helper_bridge() setter/getter to
> > > ArcServiceManager.
> > > > > > > * Add arc_intent_helper_bridge_ raw pointer (member variable) to
> > > > > > > ArcServiceManager;
> > > > > > > * Call
ArcServiceManager::Get()->set_arc_intent_helper_bridge(this)
> in
> > > > > > > ArcIntentHelperBridge's ctor, and
> > > > > > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in
> its
> > > > dtor.
> > > > > > > * In chrome/browser/, get an ArcIntentHelperBridge instance with
> > > > > > > ArcServiceManager::Get()->arc_intent_helper_bridge().
> > > > > > > 
> > > > > > > ArcServiceManager is already a singleton and has Get() method. The
> > > manager
> > > > > > also
> > > > > > > has getters like icon_loader() for allowing chrome/browser/ code
to
> > use
> > > > > > > components/arc/'s objects. The proposal above follows the current
> way
> > to
> > > > > some
> > > > > > > extent.
> > > > > > 
> > > > > > 
> > > > > > > * Call
ArcServiceManager::Get()->set_arc_intent_helper_bridge(this)
> in
> > > > > > ArcIntentHelperBridge's ctor, and
> > > > > > ArcServiceManager::Get()->set_arc_intent_helper_bridge(nullptr) in
its
> > > dtor.
> > > > > > 
> > > > > > I do not want this. If we'll get rid of global ptr, I prefer calling
> the
> > > > > setter
> > > > > > in ArcServiceLauncher::Initialize() instead.
> > > > > > 
> > > > > > > ArcServiceManager is already a singleton and has Get() method. The
> > > manager
> > > > > > also
> > > > > > > has getters like icon_loader() for allowing chrome/browser/ code
to
> > use
> > > > > > > components/arc/'s objects. The proposal above follows the current
> way
> > to
> > > > > some
> > > > > > > extent.
> > > > > > 
> > > > > > We have some getters, but those are not for ArcService classes, so
> this
> > > > looks
> > > > > > something new to me, too. (And that's why I recommended to oka@ san
to
> > use
> > > > > > global ptr.)
> > > > > 
> > > > > Well, I wanted to make the code *look* slightly better (although it
does
> > not
> > > > > resolve anything and the member variable is still effectively like a
> > global
> > > > > pointer) but if it's not for your taste, that's fine :)
> > > > > 
> > > > > BTW, I have another idea to remove g_arc_intent_helper_bridge (this
> time,
> > > > > really) and confirmed that hidehiko@ is okay with it. The idea is:
> > > > > 
> > > > > * Continue to define the Observer class, but probably in a dedicated
> file
> > in
> > > > > components/arc/intent_helper/.
> > > > > * Add AddObserver and RemoveObserver to ArcIntentHelperBridge.
> > > > > * Let ArcServiceManager observe ArcIntentHelperBridge. You can call
> > > > > ArcIntentHelperBridge::AddObserver(arc_service_manager); in
> > > > > ArcServiceLauncher::Initialize() where ArcIntentHelperBridge is
created
> > and
> > > > > added to ArcServiceManager.
> > > > > * Since ArcServiceManager always outlives the ArcIntentHelperBridge
> > object,
> > > > > ArcServiceManager doesn't have to call
> > > > > ArcIntentHelperBridge::RemoveObserver(this); (we need some code
comments
> > > > > though.)
> > > > > * Let your EventRouter code observe ArcServiceManager, instead of
> > > > > ArcIntentHelperBridge. You can call ArcServiceManager::AddObserver()
in
> > > > > EventRouter::ObserveEvents().
> > > > > * You can also call ArcServiceManager::RemoveObserver() in
> > > > > EventRouter::Shutdown(). When EventRouter::Shutdown() is called,
> > > > > ArcIntentHelperBridge has already been gone, but ArcServiceManager is
> > still
> > > > > there.
> > > > > * Obviously, you also have to chain the OnAppsUpdated() calls.
> > > > > ArcIntentHelperBridge calls its observers' OnAppsUpdated(), which is
> ASM's
> > > > > OnAppsUpdated(). ASM's OnAppsUpdated() calls its observers'
> > OnAppsUpdated(),
> > > > > which is EventRouter::OnAppsUpdated().
> > > > > 
> > > > > WDYT?
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > Thank you for the suggestion. I will try the construct in the next
patch.
> > > 
> > > What do you mean by "the next patch"? Next patch set in this CL, or
> different
> > > CL? Since this is not a small TODO but more like a structural change, I'd
> > prefer
> > > the former unless this is urgent.
> > 
> > Updated the construct as you suggested.
> > 
> >
>
https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/extensions/file_manager/event_router.cc
(right):
> > 
> >
>
https://codereview.chromium.org/2487623002/diff/160001/chrome/browser/chromeo...
> > chrome/browser/chromeos/extensions/file_manager/event_router.cc:416:
> > bridge->RemoveObserver(this);
> > On 2016/11/21 17:21:05, Yusuke Sato wrote:
> > > How did you test this? |bridge| might be NULL.
> > 
> > Test procedure is written in https://codereview.chromium.org/2513493002/
> > I copied it in this CL's description.
> > 
> > I updated the construction to chain OnAppsUpdated.
> > 
> >
>
https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
> > File components/arc/intent_helper/arc_intent_helper_bridge.h (right):
> > 
> >
>
https://codereview.chromium.org/2487623002/diff/160001/components/arc/intent_...
> > components/arc/intent_helper/arc_intent_helper_bridge.h:52: // OnAppsUpdated
> is
> > called when intent helper is updated.
> > On 2016/11/21 17:21:05, Yusuke Sato wrote:
> > > // Called when app's intent filter is updated.
> > > 
> > > ?
> > 
> > Done.
> 
> I will perform the following test tomorrow:
> Manually tested using Minnie with this CL 
> https://codereview.chromium.org/2513493002/:
> 1. Select a PDF file on Files App.
> 2. Install Kindle.
> 3. Confirm "more action..." appears on the context menu and Open
> button's UI changes to be able to open Kindle.

PTAL. I've done the test.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/260001

4 years ago (2016-11-25 11:54:28 UTC) #40

commit-bot: I haz the power

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

4 years ago (2016-11-25 12:22:28 UTC) #41

hidehiko

Almost looks good! I only have minor comments. https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/arc/arc_service_launcher.cc File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/arc/arc_service_launcher.cc#newcode6 chrome/browser/chromeos/arc/arc_service_launcher.cc:6: #include ...

4 years ago (2016-11-25 15:38:00 UTC) #43

hidehiko

Oops, sorry, I overlooked one more stuff... https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode405 chrome/browser/chromeos/extensions/file_manager/event_router.cc:405: BroadcastEvent(profile_, Even ...

4 years ago (2016-11-25 17:10:44 UTC) #44

oka

https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/arc/arc_service_launcher.cc File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/arc/arc_service_launcher.cc#newcode6 chrome/browser/chromeos/arc/arc_service_launcher.cc:6: On 2016/11/25 15:38:00, hidehiko wrote: > #include <utility> for ...

4 years ago (2016-11-28 07:15:43 UTC) #45

oka

On 2016/11/28 07:15:43, oka wrote: > https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/arc/arc_service_launcher.cc > File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): > > https://codereview.chromium.org/2487623002/diff/260001/chrome/browser/chromeos/arc/arc_service_launcher.cc#newcode6 > ...

4 years ago (2016-11-28 07:30:54 UTC) #46

Yusuke Sato

(drive-by, will take a look in detail tomorrow.) https://codereview.chromium.org/2487623002/diff/280001/components/arc/intent_helper/arc_intent_helper_observer.h File components/arc/intent_helper/arc_intent_helper_observer.h (right): https://codereview.chromium.org/2487623002/diff/280001/components/arc/intent_helper/arc_intent_helper_observer.h#newcode17 components/arc/intent_helper/arc_intent_helper_observer.h:17: };

4 years ago (2016-11-28 07:48:47 UTC) #47

oka

On 2016/11/28 07:48:47, Yusuke Sato wrote: > (drive-by, will take a look in detail tomorrow.) ...

4 years ago (2016-11-28 08:02:07 UTC) #48

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/280001

4 years ago (2016-11-28 09:15:51 UTC) #50

commit-bot: I haz the power

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

4 years ago (2016-11-28 09:26:46 UTC) #51

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/241722)

4 years ago (2016-11-28 09:26:47 UTC) #52

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/320001

4 years ago (2016-11-28 09:54:39 UTC) #54

oka

https://codereview.chromium.org/2487623002/diff/260001/components/arc/intent_helper/arc_intent_helper_bridge.cc File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2487623002/diff/260001/components/arc/intent_helper/arc_intent_helper_bridge.cc#newcode177 components/arc/intent_helper/arc_intent_helper_bridge.cc:177: for (auto& observer : observer_list_) On 2016/11/28 07:15:43, oka ...

4 years ago (2016-11-28 09:57:02 UTC) #55

commit-bot: I haz the power

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

4 years ago (2016-11-28 10:26:24 UTC) #56

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/280219)

4 years ago (2016-11-28 10:26:25 UTC) #57

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/340001

4 years ago (2016-11-28 13:21:31 UTC) #59

commit-bot: I haz the power

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

4 years ago (2016-11-28 14:47:13 UTC) #61

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/322698)

4 years ago (2016-11-28 14:47:14 UTC) #62

Yusuke Sato

lgtm with nits, thanks for addressing all the comments. Defer to other owners in TOK. ...

4 years ago (2016-11-28 19:24:54 UTC) #63

Yusuke Sato

https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode415 chrome/browser/chromeos/extensions/file_manager/event_router.cc:415: if (arc::ArcSessionManager::IsAllowedForProfile(profile_)) On 2016/11/28 19:24:53, Yusuke Sato wrote: > ...

4 years ago (2016-11-29 01:26:40 UTC) #64

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/360001

4 years ago (2016-11-29 06:49:20 UTC) #66

oka

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years ago (2016-11-29 06:54:18 UTC) #67

oka

https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode415 chrome/browser/chromeos/extensions/file_manager/event_router.cc:415: if (arc::ArcSessionManager::IsAllowedForProfile(profile_)) On 2016/11/29 01:26:40, Yusuke Sato wrote: > ...

4 years ago (2016-11-29 06:56:49 UTC) #68

commit-bot: I haz the power

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

4 years ago (2016-11-29 07:21:34 UTC) #69

oka

On 2016/11/29 11:02:30, kinaba wrote: > lgtm from my side, too. Thank you! I'll commit ...

4 years ago (2016-11-29 11:18:01 UTC) #72

hidehiko

Minor comments only. (Sorry for delay). https://codereview.chromium.org/2487623002/diff/360001/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc File components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc (right): https://codereview.chromium.org/2487623002/diff/360001/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc#newcode22 components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc:22: : icon_loader_(new ActivityIconLoader()), ...

4 years ago (2016-11-29 15:29:30 UTC) #73

oka

PTAL. https://codereview.chromium.org/2487623002/diff/360001/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc File components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc (right): https://codereview.chromium.org/2487623002/diff/360001/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc#newcode22 components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc:22: : icon_loader_(new ActivityIconLoader()), On 2016/11/29 15:29:30, hidehiko wrote: ...

4 years ago (2016-11-30 07:22:16 UTC) #74

oka

The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org, kinaba@chromium.org Link to ...

4 years ago (2016-11-30 09:23:13 UTC) #77

commit-bot: I haz the power

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

4 years ago (2016-11-30 09:23:29 UTC) #78

commit-bot: I haz the power

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

4 years ago (2016-11-30 09:26:04 UTC) #79

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/270904) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, ...

4 years ago (2016-11-30 09:26:06 UTC) #80

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/400001

4 years ago (2016-11-30 09:45:27 UTC) #82

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/420001

4 years ago (2016-11-30 09:48:53 UTC) #84

oka

The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, yusukes@chromium.org, kinaba@chromium.org Link ...

4 years ago (2016-11-30 09:54:09 UTC) #86

commit-bot: I haz the power

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

4 years ago (2016-11-30 09:54:24 UTC) #87

commit-bot: I haz the power

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

4 years ago (2016-11-30 10:00:37 UTC) #89

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/315269)

4 years ago (2016-11-30 10:00:39 UTC) #90

oka

+isherman@ for extensions/browser/extension_event_histogram_value.h +mtomasz@ for chrome/common/extensions/api/file_manager_private.idl

4 years ago (2016-11-30 11:29:45 UTC) #92

Ilya Sherman

https://codereview.chromium.org/2487623002/diff/420001/extensions/browser/extension_event_histogram_value.h File extensions/browser/extension_event_histogram_value.h (right): https://codereview.chromium.org/2487623002/diff/420001/extensions/browser/extension_event_histogram_value.h#newcode426 extensions/browser/extension_event_histogram_value.h:426: FILE_MANAGER_PRIVATE_ON_APPS_UPDATED, Please update histograms.xml, as described two lines below ...

4 years ago (2016-11-30 23:08:10 UTC) #95

oka

https://codereview.chromium.org/2487623002/diff/420001/extensions/browser/extension_event_histogram_value.h File extensions/browser/extension_event_histogram_value.h (right): https://codereview.chromium.org/2487623002/diff/420001/extensions/browser/extension_event_histogram_value.h#newcode426 extensions/browser/extension_event_histogram_value.h:426: FILE_MANAGER_PRIVATE_ON_APPS_UPDATED, On 2016/11/30 23:08:09, Ilya Sherman wrote: > Please ...

4 years ago (2016-12-01 07:05:46 UTC) #96

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/440001

4 years ago (2016-12-01 12:27:57 UTC) #99

commit-bot: I haz the power

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

4 years ago (2016-12-01 14:29:02 UTC) #100

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/325217)

4 years ago (2016-12-01 14:29:04 UTC) #101

oka

The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, yusukes@chromium.org, kinaba@chromium.org Link ...

4 years ago (2016-12-01 15:00:51 UTC) #103

commit-bot: I haz the power

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

4 years ago (2016-12-01 15:01:02 UTC) #104

commit-bot: I haz the power

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

4 years ago (2016-12-01 15:09:10 UTC) #105

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/316370)

4 years ago (2016-12-01 15:09:12 UTC) #106

Daniel Erat

ilya, mind taking a look for these files? extensions/browser/extension_event_histogram_value.h tools/metrics/histograms/histograms.xml

4 years ago (2016-12-01 18:04:32 UTC) #108

Daniel Erat

derat@chromium.org changed reviewers: + mpearson@chromium.org

4 years ago (2016-12-02 00:17:08 UTC) #109

Daniel Erat

mark, do you mind reviewing this for these files? extensions/browser/extension_event_histogram_value.h tools/metrics/histograms/histograms.xml

4 years ago (2016-12-02 00:17:10 UTC) #110

Daniel Erat

thanks! sending to the CQ again since i'm eager for this to go in. :-)

4 years ago (2016-12-02 00:37:42 UTC) #112

commit-bot: I haz the power

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

4 years ago (2016-12-02 00:38:37 UTC) #114

commit-bot: I haz the power

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

4 years ago (2016-12-02 01:10:43 UTC) #116

oka

https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode415 chrome/browser/chromeos/extensions/file_manager/event_router.cc:415: if (arc::ArcSessionManager::IsAllowedForProfile(profile_)) On 2016/11/29 06:56:48, oka wrote: > On ...

4 years ago (2016-12-02 02:39:04 UTC) #117

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/460001

4 years ago (2016-12-02 02:43:48 UTC) #119

oka

I added the line back. Let's see what try says. On Fri, Dec 2, 2016 ...

4 years ago (2016-12-02 02:44:25 UTC) #120

Yusuke Sato

On 2016/12/02 02:44:25, oka wrote: > I added the line back. Let's see what try ...

4 years ago (2016-12-02 02:51:54 UTC) #121

Yeah I know. I just wanted to run try before I go lunch.
Do you have any idea what the proper fix is?

On Fri, Dec 2, 2016 at 11:51 AM <yusukes@chromium.org> wrote:

On 2016/12/02 02:44:25, oka wrote:
> I added the line back. Let's see what try says.
>
> On Fri, Dec 2, 2016 at 11:39 AM <mailto:oka@chromium.org> wrote:
>
> >
> >
> >
>
https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/extensions/file_manager/event_router.cc
> > (right):
> >
> >
> >
>
https://codereview.chromium.org/2487623002/diff/340001/chrome/browser/chromeo...
> > chrome/browser/chromeos/extensions/file_manager/event_router.cc:415: if
> > (arc::ArcSessionManager::IsAllowedForProfile(profile_))
> > On 2016/11/29 06:56:48, oka wrote:
> > > On 2016/11/29 01:26:40, Yusuke Sato wrote:
> > > > On 2016/11/28 19:24:53, Yusuke Sato wrote:
> > > > > This line is causing the try failures. Please check.
> > > >
> > > > FYI, removing an unknown observer from base::ObserverList is always
> > no-op (and
> > > I
> > > > think it's okay to assume that AddObserver/RemoveObserver methods in
> > Chromium
> > > > are backed by the base class unless otherwise noted. You could
> > explicitly
> > > > mention that in arc_session_manager.h though.) So I think it's safe
> > to just
> > > > remove L415.
> > > >
> > > Done. Thanks!
> >
> > Looks like to line is actually needed to get BrowserTest.Title pass.
> >
> > https://codereview.chromium.org/2487623002/
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I think resurrecting the line will reintroduce this failure:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

Do you know how to build tests locally for your goobuntu workstation
(outside
the simplechrome shell) and run them with --gtest_filter? You should
probably do
that for faster iteration.



https://codereview.chromium.org/2487623002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

kinaba

https://codereview.chromium.org/2487623002/diff/460001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/460001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode416 chrome/browser/chromeos/extensions/file_manager/event_router.cc:416: arc::ArcServiceManager::Get()->RemoveObserver(this); * EventRounter::Shutdown() is called inside the destructor of ...

4 years ago (2016-12-02 04:04:26 UTC) #123

commit-bot: I haz the power

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

4 years ago (2016-12-02 04:15:35 UTC) #124

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/325871)

4 years ago (2016-12-02 04:15:37 UTC) #125

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/480001

4 years ago (2016-12-02 04:52:50 UTC) #127

oka

https://codereview.chromium.org/2487623002/diff/460001/chrome/browser/chromeos/extensions/file_manager/event_router.cc File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/2487623002/diff/460001/chrome/browser/chromeos/extensions/file_manager/event_router.cc#newcode416 chrome/browser/chromeos/extensions/file_manager/event_router.cc:416: arc::ArcServiceManager::Get()->RemoveObserver(this); On 2016/12/02 04:04:25, kinaba wrote: > * EventRounter::Shutdown() ...

4 years ago (2016-12-02 04:59:09 UTC) #128

commit-bot: I haz the power

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

4 years ago (2016-12-02 05:58:35 UTC) #129

oka

The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, mtomasz@chromium.org, hidehiko@chromium.org, yusukes@chromium.org, ...

4 years ago (2016-12-02 05:59:03 UTC) #132

commit-bot: I haz the power

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

4 years ago (2016-12-02 05:59:26 UTC) #133

kinaba

On 2016/12/02 05:59:26, commit-bot: I haz the power wrote: > CQ is trying da patch. ...

4 years ago (2016-12-02 06:00:27 UTC) #134

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487623002/480001

4 years ago (2016-12-02 06:02:41 UTC) #137

oka

On 2016/12/02 06:00:27, kinaba wrote: > On 2016/12/02 05:59:26, commit-bot: I haz the power wrote: ...

4 years ago (2016-12-02 06:04:53 UTC) #138

oka

On 2016/12/02 06:00:27, kinaba wrote: > On 2016/12/02 05:59:26, commit-bot: I haz the power wrote: ...

4 years ago (2016-12-02 06:04:58 UTC) #139

commit-bot: I haz the power

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

4 years ago (2016-12-02 06:06:45 UTC) #140

Yusuke Sato

On 2016/12/02 06:04:58, oka wrote: > On 2016/12/02 06:00:27, kinaba wrote: > > On 2016/12/02 ...

4 years ago (2016-12-02 06:09:17 UTC) #142

oka

On 2016/12/02 06:09:17, Yusuke Sato wrote: > On 2016/12/02 06:04:58, oka wrote: > > On ...

4 years ago (2016-12-02 06:11:39 UTC) #143

commit-bot: I haz the power

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

4 years ago (2016-12-02 06:12:19 UTC) #145

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 480001, "attempt_start_ts": 1480659110248280, "parent_rev": "4d79e926d837cb1ea5b01905631925e6fbaee7a1", "commit_rev": "f2d5bf352d3a126d73487f24f31c638f0e9ce1aa"}

4 years ago (2016-12-02 06:16:37 UTC) #146

commit-bot: I haz the power

Description was changed from ========== Notify Files App when ARC++ app is installed/removed. This change ...

4 years ago (2016-12-02 06:22:24 UTC) #148

commit-bot: I haz the power

4 years ago (2016-12-02 06:22:29 UTC) #149

Message was sent while issue was closed.

Patchset 25 (id:??) landed as
https://crrev.com/948665d2c9c7f3238ab22e17a44df727a76f747d
Cr-Commit-Position: refs/heads/master@{#435868}

Issue 2487623002: Notify Files App when ARC++ app is installed/removed (Closed)

Description

Patch Set 1 #

Patch Set 2 : Notify Files App when ARC++ app is updated. #

Patch Set 3 : Notify Files App when ARC++ app is installed/removed. #

Patch Set 4 : Address comments. #

Patch Set 5 : rebase #

Patch Set 6 : Fix base. #

Patch Set 7 : Added comments to Observer and Get. #

Patch Set 8 : nit: OnAppsUpdated -> onAppsUpdated in .idl #

Patch Set 9 : Add RemoverObserver. #

Patch Set 10 : Update a comment. #

Patch Set 11 : wip: updated the construct to chain OnAppsUpdated. #

Patch Set 12 : Removed global pointer in arc_intent_helper_bridge and separeted observer class out. #

Patch Set 13 : Added observer file for real. #

Patch Set 14 : Bug fix: I forgot to add intent_helper to arc_service_manager. #

Patch Set 15 : Addressed some comments. #

Patch Set 16 : Fixed compile and lint errors and added a test which at least compile. I couldn't figure out how to… #

Patch Set 17 : git cl format #

Patch Set 18 : Updated the test to pass. #

Patch Set 19 : Addressed comments. #

Patch Set 20 : Addressed comments. #

Patch Set 21 : Rebased. #

Patch Set 22 : Removed accidentally added function while rebasing. #

Patch Set 23 : Updated histograms.xml. #

Patch Set 24 : Fixed BrowserTest.Title. #

Patch Set 25 : Add IsInitialized to arc_service_manager. #

Messages