Bug 119095
| Summary: | Web Inspector: fix inconsistent use of 0-based and 1-based line numbers | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Brian Burg <burg> |
| Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | fpizlo, ggaren, graouts, joepeck, mark.lam, oliver, timothy, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
Brian Burg
Throughout the backend there are ugly and confusing mixes of 0-based an 1-based line numbers. I propose standardizing on 0-based line numbers, and only changing the ordinal base (to 1-based) at the very outer layer of the frontend. This is mostly how things work now, but there are unnecessary exceptions.
Some particularly troublesome spots:
* ScriptDebugServer - This whole file is a rat nest. It's not documented whether 0-based or 1-based line numbers are used to index breakpoints per-line. hasBreakpoint() is scary and manually changes ordinal bases.
* Timeline event line numbers are 1-based in the inspector protocol.
* ConsoleMessage stack trace line numbers are 1-based.
Chime in if there are more places to fix.
Fixing:
* If line numbers must be used as keys, then use an OrdinalNumber instead of 0- or 1-based line numbers.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Radar WebKit Bug Importer
<rdar://problem/14549211>
Brian Burg
To clarify, JSC has standardized on 1-based lines and columns; I was proposing to standardize what the inspector uses, but not necessarily what JSC does.
For things like ScriptDebugServer, I think it makes sense to correct or introduce uses of OrdinalNumber, and document places where separate line/col fields are necessary.
Joseph Pecoraro
Speaking of this, it looks like ScriptDebugServer had a bug after JSC changed to 1 based column #s:
<https://webkit.org/b/120334> Web Inspector: Column Breakpoint not working, may be off by 1
Brian Burg
This was fixed in recent refactorings of the backend.