[DevTools] Rework Popover API

[DevTools] Rework Popover API

- Client now provides a method which resolves event to UI.PopoverRequest.
- We can be sure that show/hide methods are called in pairs.
- We can now scope additional work and local variables instead of passing them strangely through the AnchorBox.

BUG=none

Review-Url: https://codereview.chromium.org/2747553002
Cr-Commit-Position: refs/heads/master@{#458663}
Committed: https://chromium.googlesource.com/chromium/src/+/cb6ebed7c921e667425703651130450eab318721

[dgozman, 2017-03-14 00:14:32]

Description was changed from

==========
popover-api-2

BUG=
==========

to

==========
[DevTools] Rework Popover API

- Client now provides a method which resolves event to UI.PopoverContent.
- We can be sure that show/hide methods are called in pairs.
- We can now scope additional work and local variables instead of passing them
strangely through the AnchorBox.

BUG=none
==========

[dgozman, 2017-03-14 00:15:28]

dgozman@chromium.org changed reviewers:
+ lushnikov@chromium.org

[dgozman, 2017-03-14 00:15:29]

Could you please take a look?

https://codereview.chromium.org/2747553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Popover.js (right):

https://codereview.chromium.org/2747553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:236: /** @typedef
{{box: !AnchorBox, show:(function(!UI.GlassPane):!Promise<boolean>),
hide:(function()|undefined)}} */
I'm thinking about onShowPopover and onHidePopover methods for better
discoverability, but convinced myself yet.

[dgozman, 2017-03-14 00:15:33]

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

[commit-bot: I haz the power, 2017-03-14 00:16:10]

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

[commit-bot: I haz the power, 2017-03-14 00:33:28]

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

[commit-bot: I haz the power, 2017-03-14 00:33:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lushnikov, 2017-03-14 01:44:04]

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js:541:
this._loadDimensionsForNode(
the _loadDimensionsForNode is only used for a popover. Can we promisify it?

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:36:
this._popoverHelper = new UI.PopoverHelper(this.element, false,
this._getPopoverContent.bind(this));
nit: could you please add /* disableOnClick */ comment for the "false" argument

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:628:
target.runtimeModel.releaseObjectGroup('popover');
these partial clean-ups would be easy to forget. Maybe we should call hide() for
every call to show(), regardless of show's return value?

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:389:
* @return {?UI.PopoverContent}
It feels like client should create actual popover DOM

Let's call this UI.PopoverRequest, wdyt?

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:391:
_getErrorPopoverContent(event) {
_maybeShowErrorPopover?

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:455:
target.runtimeModel.releaseObjectGroup('popover');
objectPopoverHelper.dispose();

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Popover.js (left):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:201:
console.error('One popover is already visible');
let's have this back

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Popover.js (right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:151: if (event.which
&& this._disableOnClick)
move to callsites

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:200:
content.show.call(null, popover).then(success => {
lets .catch here

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:202: return;
if (success) <hide>

[dgozman, 2017-03-14 21:34:02]

Please take another look.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js:541:
this._loadDimensionsForNode(
On 2017/03/14 01:44:03, lushnikov wrote:
> the _loadDimensionsForNode is only used for a popover. Can we promisify it?

Done.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:36:
this._popoverHelper = new UI.PopoverHelper(this.element, false,
this._getPopoverContent.bind(this));
On 2017/03/14 01:44:03, lushnikov wrote:
> nit: could you please add /* disableOnClick */ comment for the "false"
argument

Turned into a method.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:628:
target.runtimeModel.releaseObjectGroup('popover');
On 2017/03/14 01:44:03, lushnikov wrote:
> these partial clean-ups would be easy to forget. Maybe we should call hide()
for
> every call to show(), regardless of show's return value?

I think that would be a strange API and will not deliver better code.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:389:
* @return {?UI.PopoverContent}
On 2017/03/14 01:44:04, lushnikov wrote:
> It feels like client should create actual popover DOM
> 
> Let's call this UI.PopoverRequest, wdyt?

Done.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:455:
target.runtimeModel.releaseObjectGroup('popover');
On 2017/03/14 01:44:04, lushnikov wrote:
> objectPopoverHelper.dispose();

Done.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Popover.js (left):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:201:
console.error('One popover is already visible');
On 2017/03/14 01:44:04, lushnikov wrote:
> let's have this back

Done.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Popover.js (right):

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:151: if (event.which
&& this._disableOnClick)
On 2017/03/14 01:44:04, lushnikov wrote:
> move to callsites

Done.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:200:
content.show.call(null, popover).then(success => {
On 2017/03/14 01:44:04, lushnikov wrote:
> lets .catch here

Turned out that we don't have to cleanup anything here and we don't block
anything as well.

https://codereview.chromium.org/2747553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:202: return;
On 2017/03/14 01:44:04, lushnikov wrote:
> if (success) <hide>

Done.

[dgozman, 2017-03-14 21:34:06]

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

[commit-bot: I haz the power, 2017-03-14 21:34:40]

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

[commit-bot: I haz the power, 2017-03-14 21:44:45]

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

[commit-bot: I haz the power, 2017-03-14 21:44:46]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[lushnikov, 2017-03-14 22:13:44]

lgtm

https://codereview.chromium.org/2747553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Popover.js (right):

https://codereview.chromium.org/2747553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:210: if
(request.hide)
having .hide mandatory would simplify things

[lushnikov, 2017-03-14 22:13:50]

lgtm

[dgozman, 2017-03-14 22:51:25]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2017-03-14 22:52:31]

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

[commit-bot: I haz the power, 2017-03-14 23:05:24]

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

[commit-bot: I haz the power, 2017-03-14 23:05:26]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dgozman, 2017-03-14 23:10:57]

Description was changed from

==========
[DevTools] Rework Popover API

- Client now provides a method which resolves event to UI.PopoverContent.
- We can be sure that show/hide methods are called in pairs.
- We can now scope additional work and local variables instead of passing them
strangely through the AnchorBox.

BUG=none
==========

to

==========
[DevTools] Rework Popover API

- Client now provides a method which resolves event to UI.PopoverRequest.
- We can be sure that show/hide methods are called in pairs.
- We can now scope additional work and local variables instead of passing them
strangely through the AnchorBox.

BUG=none
==========

[dgozman, 2017-03-14 23:35:13]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:36:03]

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

[commit-bot: I haz the power, 2017-03-14 23:49:12]

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

[commit-bot: I haz the power, 2017-03-14 23:49:13]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dgozman, 2017-03-15 23:18:39]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2017-03-15 23:20:01]

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

[commit-bot: I haz the power, 2017-03-15 23:31:10]

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

[commit-bot: I haz the power, 2017-03-15 23:31:12]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dgozman, 2017-03-22 01:46:12]

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

[commit-bot: I haz the power, 2017-03-22 01:47:10]

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

[commit-bot: I haz the power, 2017-03-22 03:49:10]

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

[commit-bot: I haz the power, 2017-03-22 03:49:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[dgozman, 2017-03-22 05:28:07]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-03-22 05:28:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2747553002/#ps100001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-22 05:28:39]

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

[commit-bot: I haz the power, 2017-03-22 07:23:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490160487365760,
"parent_rev": "ba66233b874fd4f9faac1feed4dbe1a83e6458ed", "commit_rev":
"cb6ebed7c921e667425703651130450eab318721"}

[commit-bot: I haz the power, 2017-03-22 07:24:04]

Description was changed from

==========
[DevTools] Rework Popover API

- Client now provides a method which resolves event to UI.PopoverRequest.
- We can be sure that show/hide methods are called in pairs.
- We can now scope additional work and local variables instead of passing them
strangely through the AnchorBox.

BUG=none
==========

to

==========
[DevTools] Rework Popover API

- Client now provides a method which resolves event to UI.PopoverRequest.
- We can be sure that show/hide methods are called in pairs.
- We can now scope additional work and local variables instead of passing them
strangely through the AnchorBox.

BUG=none

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

[commit-bot: I haz the power, 2017-03-22 07:24:06]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/cb6ebed7c921e667425703651130...