Fix some non-aura specific code related to metro startup.

Fix some non-aura specific code related to metro startup.

There was a legitimate bug here, but it only occurred on non-aura builds.
On aura builds, the code was still incorrect but by luck exhibited the
correct behavior.

The confusion about this behavior comes from the fact that prior to
r232828, we allowed Desktop and Metro instances of chrome to run 
simultaneously, depending on which shortcut you clicked.  As of r232828, 
all shortcuts lead to the same browser experience, and we store the 
"stickiness" in the registry and the launch experience is based entirely 
off of the value stored in the registry.

As a result, the code here which was incorrect, and which is fixed in this 
patch, only matters for non-Aura.


BUG=314034
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233527

[rvargas (doing something else), 2013-11-05 21:07:29]

LGTM after adding a comment. Thanks.

https://codereview.chromium.org/60823002/diff/1/chrome/browser/chrome_process...
File chrome/browser/chrome_process_finder_win.cc (right):

https://codereview.chromium.org/60823002/diff/1/chrome/browser/chrome_process...
chrome/browser/chrome_process_finder_win.cc:145: if
(base::win::IsProcessImmersive(process_handle.Get()))
A short comment about not mixing the two if statements is in order here.

[sky, 2013-11-05 21:13:52]

LGTM

[commit-bot: I haz the power, 2013-11-05 21:49:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zturner@chromium.org/60823002/70001

[gab, 2013-11-05 22:46:55]

https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
File chrome/browser/chrome_process_finder_win.cc (right):

https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
chrome/browser/chrome_process_finder_win.cc:145: // Receive() causes the process
handle to be set in the destructor of the
This very much surprised me as I expected the object returned by Receive() to
only live in the scope of base::OpenProcessHandleWithAccess()... so I wrote some
code to verify...

class Foo {
 public:
  Foo() { cout << "constructed foo\n"; }
  ~Foo() {cout << "destructed foo\n"; }
};

bool DoSomethingWithFoo(const Foo& foo) {
  cout << "doing the foo dance\n";
  return true;
}

bool DoSomethingElse() {
  cout << "sad in my bar with no foo\n";
  return true;
}

int _tmain(int argc, wchar_t* argv[])
{
  cout << "warming up\n";
  if (DoSomethingWithFoo(Foo()) &&
      DoSomethingElse()) {
    cout << "done\n";
    return 1;
  }
  return 0;
}

.....

And the output was:
warming up
constructed foo
doing the foo dance
sad in my bar with no foo
destructed foo
done

Showing that the scope of Foo() is indeed the entire if evaluation scope, not
only DoSomethingWithFoo()... very weird... I almost feel this is an MSVC bug...
worth trying on a Linux box with clank if someone has one handy I'd say.

https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
chrome/browser/chrome_process_finder_win.cc:148: // as part of a separate if
statement.
This is pretty bad... a comment should probably be added to
process_handle.Receive()... even better, this should use a different method as I
don't feel this erroneous usage pattern is uncommon...

https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
chrome/browser/chrome_process_finder_win.cc:150: chrome::ActivateMetroChrome();
The use case this was meant to handle is that if you launch chrome.exe from the
command-line (and thus bypass delegate_execute.exe's logic to decide Metro vs
Desktop) we still want to rendez-vous with the Metro Chrome (in singleton mode
we have had no choice...).

Now.. in Aura.. if the only active desktop is Ash; should we activate here
instead of opening a native desktop window? We should do whatever
delegate_execute.exe does in the regular case IMO.

[gab, 2013-11-05 22:50:11]

CC some C++/compiler experts, see below

https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
File chrome/browser/chrome_process_finder_win.cc (right):

https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
chrome/browser/chrome_process_finder_win.cc:145: // Receive() causes the process
handle to be set in the destructor of the
On 2013/11/05 22:46:55, gab wrote:
> This very much surprised me as I expected the object returned by Receive() to
> only live in the scope of base::OpenProcessHandleWithAccess()... so I wrote
some
> code to verify...
> 
> class Foo {
>  public:
>   Foo() { cout << "constructed foo\n"; }
>   ~Foo() {cout << "destructed foo\n"; }
> };
> 
> bool DoSomethingWithFoo(const Foo& foo) {
>   cout << "doing the foo dance\n";
>   return true;
> }
> 
> bool DoSomethingElse() {
>   cout << "sad in my bar with no foo\n";
>   return true;
> }
> 
> int _tmain(int argc, wchar_t* argv[])
> {
>   cout << "warming up\n";
>   if (DoSomethingWithFoo(Foo()) &&
>       DoSomethingElse()) {
>     cout << "done\n";
>     return 1;
>   }
>   return 0;
> }
> 
> .....
> 
> And the output was:
> warming up
> constructed foo
> doing the foo dance
> sad in my bar with no foo
> destructed foo
> done
> 
> Showing that the scope of Foo() is indeed the entire if evaluation scope, not
> only DoSomethingWithFoo()... very weird... I almost feel this is an MSVC
bug...
> worth trying on a Linux box with clank if someone has one handy I'd say.

this ^^^

[zturner, 2013-11-05 22:52:58]

On 2013/11/05 22:46:55, gab wrote:
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> File chrome/browser/chrome_process_finder_win.cc (right):
> 
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> chrome/browser/chrome_process_finder_win.cc:145: // Receive() causes the
process
> handle to be set in the destructor of the
> This very much surprised me as I expected the object returned by Receive() to
> only live in the scope of base::OpenProcessHandleWithAccess()... so I wrote
some
> code to verify...
> 
> class Foo {
>  public:
>   Foo() { cout << "constructed foo\n"; }
>   ~Foo() {cout << "destructed foo\n"; }
> };
> 
> bool DoSomethingWithFoo(const Foo& foo) {
>   cout << "doing the foo dance\n";
>   return true;
> }
> 
> bool DoSomethingElse() {
>   cout << "sad in my bar with no foo\n";
>   return true;
> }
> 
> int _tmain(int argc, wchar_t* argv[])
> {
>   cout << "warming up\n";
>   if (DoSomethingWithFoo(Foo()) &&
>       DoSomethingElse()) {
>     cout << "done\n";
>     return 1;
>   }
>   return 0;
> }
> 
> .....
> 
> And the output was:
> warming up
> constructed foo
> doing the foo dance
> sad in my bar with no foo
> destructed foo
> done
> 
> Showing that the scope of Foo() is indeed the entire if evaluation scope, not
> only DoSomethingWithFoo()... very weird... I almost feel this is an MSVC
bug...
> worth trying on a Linux box with clank if someone has one handy I'd say.
> 
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> chrome/browser/chrome_process_finder_win.cc:148: // as part of a separate if
> statement.
> This is pretty bad... a comment should probably be added to
> process_handle.Receive()... even better, this should use a different method as
I
> don't feel this erroneous usage pattern is uncommon...
> 
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> chrome/browser/chrome_process_finder_win.cc:150:
chrome::ActivateMetroChrome();
> The use case this was meant to handle is that if you launch chrome.exe from
the
> command-line (and thus bypass delegate_execute.exe's logic to decide Metro vs
> Desktop) we still want to rendez-vous with the Metro Chrome (in singleton mode
> we have had no choice...).
> 
> Now.. in Aura.. if the only active desktop is Ash; should we activate here
> instead of opening a native desktop window? We should do whatever
> delegate_execute.exe does in the regular case IMO.

(Disclaimer: I'm not a language lawyer, so I might be wrong)

I believe it is actually be correct according to the standard.  Temporaries live
until the end of the sequence point, which in this case is the end of the if
statement.  Here is the relevant line from the standard.

"Temporary objects are destroyed as the last step in evaluating the
full-expression (1.9) that (lexically) contains the point where they were
created. [12.2/3]"

[gab, 2013-11-05 23:45:09]

On 2013/11/05 22:52:58, zturner wrote:
> On 2013/11/05 22:46:55, gab wrote:
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > File chrome/browser/chrome_process_finder_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > chrome/browser/chrome_process_finder_win.cc:145: // Receive() causes the
> process
> > handle to be set in the destructor of the
> > This very much surprised me as I expected the object returned by Receive()
to
> > only live in the scope of base::OpenProcessHandleWithAccess()... so I wrote
> some
> > code to verify...
> > 
> > class Foo {
> >  public:
> >   Foo() { cout << "constructed foo\n"; }
> >   ~Foo() {cout << "destructed foo\n"; }
> > };
> > 
> > bool DoSomethingWithFoo(const Foo& foo) {
> >   cout << "doing the foo dance\n";
> >   return true;
> > }
> > 
> > bool DoSomethingElse() {
> >   cout << "sad in my bar with no foo\n";
> >   return true;
> > }
> > 
> > int _tmain(int argc, wchar_t* argv[])
> > {
> >   cout << "warming up\n";
> >   if (DoSomethingWithFoo(Foo()) &&
> >       DoSomethingElse()) {
> >     cout << "done\n";
> >     return 1;
> >   }
> >   return 0;
> > }
> > 
> > .....
> > 
> > And the output was:
> > warming up
> > constructed foo
> > doing the foo dance
> > sad in my bar with no foo
> > destructed foo
> > done
> > 
> > Showing that the scope of Foo() is indeed the entire if evaluation scope,
not
> > only DoSomethingWithFoo()... very weird... I almost feel this is an MSVC
> bug...
> > worth trying on a Linux box with clank if someone has one handy I'd say.
> > 
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > chrome/browser/chrome_process_finder_win.cc:148: // as part of a separate if
> > statement.
> > This is pretty bad... a comment should probably be added to
> > process_handle.Receive()... even better, this should use a different method
as
> I
> > don't feel this erroneous usage pattern is uncommon...
> > 
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > chrome/browser/chrome_process_finder_win.cc:150:
> chrome::ActivateMetroChrome();
> > The use case this was meant to handle is that if you launch chrome.exe from
> the
> > command-line (and thus bypass delegate_execute.exe's logic to decide Metro
vs
> > Desktop) we still want to rendez-vous with the Metro Chrome (in singleton
mode
> > we have had no choice...).
> > 
> > Now.. in Aura.. if the only active desktop is Ash; should we activate here
> > instead of opening a native desktop window? We should do whatever
> > delegate_execute.exe does in the regular case IMO.
> 
> (Disclaimer: I'm not a language lawyer, so I might be wrong)
> 
> I believe it is actually be correct according to the standard.  Temporaries
live
> until the end of the sequence point, which in this case is the end of the if
> statement.  Here is the relevant line from the standard.
> 
> "Temporary objects are destroyed as the last step in evaluating the
> full-expression (1.9) that (lexically) contains the point where they were
> created. [12.2/3]"

Ah yes was just talking about this with other here, looks like you're right,
temporaries are not bound to scope, but rather to the next sequence point (I.e.,
end of expression).

Very interesting.

I think this means the latest implementation of Receive() using a temporary
object is incorrect as this pattern is very common.

[rvargas (doing something else), 2013-11-05 23:49:27]

On 2013/11/05 23:45:09, gab wrote:
> On 2013/11/05 22:52:58, zturner wrote:
> > On 2013/11/05 22:46:55, gab wrote:
> > >
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > > File chrome/browser/chrome_process_finder_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > > chrome/browser/chrome_process_finder_win.cc:145: // Receive() causes the
> > process
> > > handle to be set in the destructor of the
> > > This very much surprised me as I expected the object returned by Receive()
> to
> > > only live in the scope of base::OpenProcessHandleWithAccess()... so I
wrote
> > some
> > > code to verify...
> > > 
> > > class Foo {
> > >  public:
> > >   Foo() { cout << "constructed foo\n"; }
> > >   ~Foo() {cout << "destructed foo\n"; }
> > > };
> > > 
> > > bool DoSomethingWithFoo(const Foo& foo) {
> > >   cout << "doing the foo dance\n";
> > >   return true;
> > > }
> > > 
> > > bool DoSomethingElse() {
> > >   cout << "sad in my bar with no foo\n";
> > >   return true;
> > > }
> > > 
> > > int _tmain(int argc, wchar_t* argv[])
> > > {
> > >   cout << "warming up\n";
> > >   if (DoSomethingWithFoo(Foo()) &&
> > >       DoSomethingElse()) {
> > >     cout << "done\n";
> > >     return 1;
> > >   }
> > >   return 0;
> > > }
> > > 
> > > .....
> > > 
> > > And the output was:
> > > warming up
> > > constructed foo
> > > doing the foo dance
> > > sad in my bar with no foo
> > > destructed foo
> > > done
> > > 
> > > Showing that the scope of Foo() is indeed the entire if evaluation scope,
> not
> > > only DoSomethingWithFoo()... very weird... I almost feel this is an MSVC
> > bug...
> > > worth trying on a Linux box with clank if someone has one handy I'd say.
> > > 
> > >
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > > chrome/browser/chrome_process_finder_win.cc:148: // as part of a separate
if
> > > statement.
> > > This is pretty bad... a comment should probably be added to
> > > process_handle.Receive()... even better, this should use a different
method
> as
> > I
> > > don't feel this erroneous usage pattern is uncommon...
> > > 
> > >
> >
>
https://codereview.chromium.org/60823002/diff/70001/chrome/browser/chrome_pro...
> > > chrome/browser/chrome_process_finder_win.cc:150:
> > chrome::ActivateMetroChrome();
> > > The use case this was meant to handle is that if you launch chrome.exe
from
> > the
> > > command-line (and thus bypass delegate_execute.exe's logic to decide Metro
> vs
> > > Desktop) we still want to rendez-vous with the Metro Chrome (in singleton
> mode
> > > we have had no choice...).
> > > 
> > > Now.. in Aura.. if the only active desktop is Ash; should we activate here
> > > instead of opening a native desktop window? We should do whatever
> > > delegate_execute.exe does in the regular case IMO.
> > 
> > (Disclaimer: I'm not a language lawyer, so I might be wrong)
> > 
> > I believe it is actually be correct according to the standard.  Temporaries
> live
> > until the end of the sequence point, which in this case is the end of the if
> > statement.  Here is the relevant line from the standard.
> > 
> > "Temporary objects are destroyed as the last step in evaluating the
> > full-expression (1.9) that (lexically) contains the point where they were
> > created. [12.2/3]"
> 
> Ah yes was just talking about this with other here, looks like you're right,
> temporaries are not bound to scope, but rather to the next sequence point
(I.e.,
> end of expression).
> 
> Very interesting.
> 
> I think this means the latest implementation of Receive() using a temporary
> object is incorrect as this pattern is very common.

yes, that's what the c++ standard says... as for the implementation of
Receive(), yes, it is broken, but the whole pattern is broken (including the
first version) and that's why I mentioned in the bug that I been meaning to
remove it completely.

[gab, 2013-11-05 23:53:49]

Okay, also we should probably merge this in M31.
On Nov 5, 2013 6:49 PM, <rvargas@chromium.org> wrote:

> On 2013/11/05 23:45:09, gab wrote:
>
>> On 2013/11/05 22:52:58, zturner wrote:
>> > On 2013/11/05 22:46:55, gab wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.org/60823002/diff/70001/
> chrome/browser/chrome_process_finder_win.cc
>
>> > > File chrome/browser/chrome_process_finder_win.cc (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/60823002/diff/70001/
> chrome/browser/chrome_process_finder_win.cc#newcode145
>
>> > > chrome/browser/chrome_process_finder_win.cc:145: // Receive() causes
>> the
>> > process
>> > > handle to be set in the destructor of the
>> > > This very much surprised me as I expected the object returned by
>> Receive()
>> to
>> > > only live in the scope of base::OpenProcessHandleWithAccess()... so I
>>
> wrote
>
>> > some
>> > > code to verify...
>> > >
>> > > class Foo {
>> > >  public:
>> > >   Foo() { cout << "constructed foo\n"; }
>> > >   ~Foo() {cout << "destructed foo\n"; }
>> > > };
>> > >
>> > > bool DoSomethingWithFoo(const Foo& foo) {
>> > >   cout << "doing the foo dance\n";
>> > >   return true;
>> > > }
>> > >
>> > > bool DoSomethingElse() {
>> > >   cout << "sad in my bar with no foo\n";
>> > >   return true;
>> > > }
>> > >
>> > > int _tmain(int argc, wchar_t* argv[])
>> > > {
>> > >   cout << "warming up\n";
>> > >   if (DoSomethingWithFoo(Foo()) &&
>> > >       DoSomethingElse()) {
>> > >     cout << "done\n";
>> > >     return 1;
>> > >   }
>> > >   return 0;
>> > > }
>> > >
>> > > .....
>> > >
>> > > And the output was:
>> > > warming up
>> > > constructed foo
>> > > doing the foo dance
>> > > sad in my bar with no foo
>> > > destructed foo
>> > > done
>> > >
>> > > Showing that the scope of Foo() is indeed the entire if evaluation
>> scope,
>> not
>> > > only DoSomethingWithFoo()... very weird... I almost feel this is an
>> MSVC
>> > bug...
>> > > worth trying on a Linux box with clank if someone has one handy I'd
>> say.
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/60823002/diff/70001/
> chrome/browser/chrome_process_finder_win.cc#newcode148
>
>> > > chrome/browser/chrome_process_finder_win.cc:148: // as part of a
>> separate
>>
> if
>
>> > > statement.
>> > > This is pretty bad... a comment should probably be added to
>> > > process_handle.Receive()... even better, this should use a different
>>
> method
>
>> as
>> > I
>> > > don't feel this erroneous usage pattern is uncommon...
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/60823002/diff/70001/
> chrome/browser/chrome_process_finder_win.cc#newcode150
>
>> > > chrome/browser/chrome_process_finder_win.cc:150:
>> > chrome::ActivateMetroChrome();
>> > > The use case this was meant to handle is that if you launch chrome.exe
>>
> from
>
>> > the
>> > > command-line (and thus bypass delegate_execute.exe's logic to decide
>> Metro
>> vs
>> > > Desktop) we still want to rendez-vous with the Metro Chrome (in
>> singleton
>> mode
>> > > we have had no choice...).
>> > >
>> > > Now.. in Aura.. if the only active desktop is Ash; should we activate
>> here
>> > > instead of opening a native desktop window? We should do whatever
>> > > delegate_execute.exe does in the regular case IMO.
>> >
>> > (Disclaimer: I'm not a language lawyer, so I might be wrong)
>> >
>> > I believe it is actually be correct according to the standard.
>>  Temporaries
>> live
>> > until the end of the sequence point, which in this case is the end of
>> the if
>> > statement.  Here is the relevant line from the standard.
>> >
>> > "Temporary objects are destroyed as the last step in evaluating the
>> > full-expression (1.9) that (lexically) contains the point where they
>> were
>> > created. [12.2/3]"
>>
>
>  Ah yes was just talking about this with other here, looks like you're
>> right,
>> temporaries are not bound to scope, but rather to the next sequence point
>>
> (I.e.,
>
>> end of expression).
>>
>
>  Very interesting.
>>
>
>  I think this means the latest implementation of Receive() using a
>> temporary
>> object is incorrect as this pattern is very common.
>>
>
> yes, that's what the c++ standard says... as for the implementation of
> Receive(), yes, it is broken, but the whole pattern is broken (including
> the
> first version) and that's why I mentioned in the bug that I been meaning to
> remove it completely.
>
> https://codereview.chromium.org/60823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-11-06 02:26:32]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-07 01:31:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zturner@chromium.org/60823002/70001

[commit-bot: I haz the power, 2013-11-07 07:30:54]

Change committed as 233527