Bug 162165

Summary:

Web Inspector: Make it easier to create a view from an existing DOM element

Product:

WebKit

Reporter:

Matt Baker <mattbaker>

Component:

Web Inspector

Assignee:

Matt Baker <mattbaker>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none

Matt Baker

Reported 2016-09-19 10:08:46 PDT

Instead of the optional `element` parameter, the View constructor should take an `elementOrIdentifier` which can be an HTMLElement or id attribute value.

Attachments
Patch (6.50 KB, patch) 2016-09-19 10:24 PDT, Matt Baker	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2016-09-19 10:09:09 PDT

<rdar://problem/28365848>

Matt Baker

Comment 2 2016-09-19 10:24:00 PDT

Created attachment 289236 [details] Patch

WebKit Commit Bot

Comment 3 2016-09-19 10:55:49 PDT

Comment on attachment 289236 [details] Patch Clearing flags on attachment: 289236 Committed r206101: <http://trac.webkit.org/changeset/206101>

WebKit Commit Bot

Comment 4 2016-09-19 10:55:52 PDT

All reviewed patches have been landed. Closing bug.

Joseph Pecoraro

Comment 5 2016-09-19 11:35:55 PDT

Comment on attachment 289236 [details] Patch This doesn't feel like an improvement to me. Previously we were guaranteed a single type element || null. Now it can be two different types with even less clarity at the call site. Was there a particular driver for this change?

Matt Baker

Comment 6 2016-09-19 11:42:13 PDT

(In reply to comment #5) > Comment on attachment 289236 [details] > Patch > > This doesn't feel like an improvement to me. Previously we were guaranteed a > single type element || null. Now it can be two different types with even > less clarity at the call site. Was there a particular driver for this change? It was done as a drive-by simplification why working on another patch. The parameter overloading idiom is used elsewhere, but I'm fine with rolling back if you think its less clear.

Joseph Pecoraro

Comment 7 2016-09-19 11:49:52 PDT

(In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 289236 [details] > > Patch > > > > This doesn't feel like an improvement to me. Previously we were guaranteed a > > single type element || null. Now it can be two different types with even > > less clarity at the call site. Was there a particular driver for this change? > > It was done as a drive-by simplification why working on another patch. The > parameter overloading idiom is used elsewhere, but I'm fine with rolling > back if you think its less clear. Yeah, I don't see this as a simplification. We should optimize for making the call site as clear as possible. And this makes it more difficult to read. Specifically, I see this and go "is that an identifier, or a class name?".

Note You need to log in before you can comment on or make changes to this bug.