Test thread equality when initiating PWEnv class

Test thread equality when initiating PWEnv class

ThreadUtils.assertOnUiIThread doesn't work on newer versions of
Android.  Instead, we should simply test thread equality to make
sure we don't create a PhysicalWebEnvironment singleton twice.

This change also renables Physical Web onboarding tests that had
been disabled due to this issue.

BUG=608872
Committed: https://crrev.com/cb7110459875ee2e30ab80fcf99ed16123c39899
Cr-Commit-Position: refs/heads/master@{#392471}

[cco3, 2016-05-06 22:50:10]

cco3@chromium.org changed reviewers:
+ mattreynolds@chromium.org

[mattreynolds, 2016-05-09 21:31:09]

lgtm

[cco3, 2016-05-09 21:40:48]

cco3@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[cco3, 2016-05-09 21:40:48]

Hi Dan, this is the result of that "assert on UI thread" issue you helped me
with last week.

[gone, 2016-05-09 22:11:54]

https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java
(right):

https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java:26:
}
The more common pattern we follow is to synchronize this particular block of
code with a lock.  The most recent example is here, which landed just a few
hours ago:

https://chromiumcodereview.appspot.com/1956403002/diff/20001/chrome/android/j...

In that case, we're not worried about what thread is actually doing the
instantiation, and just want to make sure only one is created.  Does that apply
here, as well?

[cco3, 2016-05-09 22:29:23]

Thank you!

https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java
(right):

https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java:26:
}
On 2016/05/09 22:11:54, dfalcantara wrote:
> The more common pattern we follow is to synchronize this particular block of
> code with a lock.  The most recent example is here, which landed just a few
> hours ago:
> 
>
https://chromiumcodereview.appspot.com/1956403002/diff/20001/chrome/android/j...
> 
> In that case, we're not worried about what thread is actually doing the
> instantiation, and just want to make sure only one is created.  Does that
apply
> here, as well?

It does, thanks.  I was under the impression that "synchronized" was frowned
upon in the chromium code base.

[gone, 2016-05-09 22:33:12]

lgtm

https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java
(right):

https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java:26:
}
On 2016/05/09 22:29:23, cco3 wrote:
> On 2016/05/09 22:11:54, dfalcantara wrote:
> > The more common pattern we follow is to synchronize this particular block of
> > code with a lock.  The most recent example is here, which landed just a few
> > hours ago:
> > 
> >
>
https://chromiumcodereview.appspot.com/1956403002/diff/20001/chrome/android/j...
> > 
> > In that case, we're not worried about what thread is actually doing the
> > instantiation, and just want to make sure only one is created.  Does that
> apply
> > here, as well?
> 
> It does, thanks.  I was under the impression that "synchronized" was frowned
> upon in the chromium code base.

Depends on the context... Synchronizing methods is discouraged because it locks
the whole object, but these one-off member field locks seem to be accepted more
readily.

[cco3, 2016-05-09 22:34:06]

On 2016/05/09 22:29:23, cco3 wrote:
> Thank you!
> 
>
https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java
> (right):
> 
>
https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java:26:
> }
> On 2016/05/09 22:11:54, dfalcantara wrote:
> > The more common pattern we follow is to synchronize this particular block of
> > code with a lock.  The most recent example is here, which landed just a few
> > hours ago:
> > 
> >
>
https://chromiumcodereview.appspot.com/1956403002/diff/20001/chrome/android/j...
> > 
> > In that case, we're not worried about what thread is actually doing the
> > instantiation, and just want to make sure only one is created.  Does that
> apply
> > here, as well?
> 
> It does, thanks.  I was under the impression that "synchronized" was frowned
> upon in the chromium code base.

Hm, the only other times I had tried it was in singleton initializers.

[cco3, 2016-05-09 22:34:10]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-05-09 22:34:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1955173003/#ps20001
(title: "Use synchronized code block")

[commit-bot: I haz the power, 2016-05-09 22:34:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1955173003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1955173003/20001

[gone, 2016-05-09 22:36:01]

On 2016/05/09 22:34:06, cco3 wrote:
> On 2016/05/09 22:29:23, cco3 wrote:
> > Thank you!
> > 
> >
>
https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1955173003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java:26:
> > }
> > On 2016/05/09 22:11:54, dfalcantara wrote:
> > > The more common pattern we follow is to synchronize this particular block
of
> > > code with a lock.  The most recent example is here, which landed just a
few
> > > hours ago:
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/1956403002/diff/20001/chrome/android/j...
> > > 
> > > In that case, we're not worried about what thread is actually doing the
> > > instantiation, and just want to make sure only one is created.  Does that
> > apply
> > > here, as well?
> > 
> > It does, thanks.  I was under the impression that "synchronized" was frowned
> > upon in the chromium code base.
> 
> Hm, the only other times I had tried it was in singleton initializers.

Weird.  In any case, we've got lots of examples of it in the codebase.  I
double-checked before suggesting it for everyone's sanity.

[commit-bot: I haz the power, 2016-05-09 22:37:25]

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

[commit-bot: I haz the power, 2016-05-09 22:37:27]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[cco3, 2016-05-09 22:49:40]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-05-09 22:49:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1955173003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-09 22:49:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1955173003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1955173003/40001

[commit-bot: I haz the power, 2016-05-09 23:28:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-09 23:31:10]

Description was changed from

==========
Test thread equality when initiating PWEnv class

ThreadUtils.assertOnUiIThread doesn't work on newer versions of
Android.  Instead, we should simply test thread equality to make
sure we don't create a PhysicalWebEnvironment singleton twice.

This change also renables Physical Web onboarding tests that had
been disabled due to this issue.

BUG=608872
==========

to

==========
Test thread equality when initiating PWEnv class

ThreadUtils.assertOnUiIThread doesn't work on newer versions of
Android.  Instead, we should simply test thread equality to make
sure we don't create a PhysicalWebEnvironment singleton twice.

This change also renables Physical Web onboarding tests that had
been disabled due to this issue.

BUG=608872

Committed: https://crrev.com/cb7110459875ee2e30ab80fcf99ed16123c39899
Cr-Commit-Position: refs/heads/master@{#392471}
==========

[commit-bot: I haz the power, 2016-05-09 23:31:11]

Patchset 3 (id:??) landed as
https://crrev.com/cb7110459875ee2e30ab80fcf99ed16123c39899
Cr-Commit-Position: refs/heads/master@{#392471}