Summary: | Web Inspector: Tabs: Conflicts with multiple Formatters per SourceCode | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, rniwa, timothy, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2015-05-06 15:17:43 PDT
This causes issues when there are multiple TextEditors with some features applied (pretty printing, Type Profiler, searching, editing). We should really reduce this duplication. Also bad, this affects SourceCodeLocation links and jump to location when there are multiple editors with different formatted/unformatted states. E.g. Timeline showing a content view for a resource that a background tab already had formatted => jumps to 0:0 because it didn't understand the incoming location was already formatted. Created attachment 271128 [details]
[PATCH] Proposed Fix
Technically an issue with multiple Formatters can still exist:
1. Open Resources tab
2. Open jquery.js
3. Close Resources tab
4. Open / Jump to Location for jquery.js
=> possible problem
But this change is a big leap forward in the common case. Closing tabs presents all kinds of other problems we perform poorly at anyways.
Comment on attachment 271128 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271128&action=review > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:87 > - // contentViews on the real representedObject and not the one originally supplied. > + // contentView on the real representedObject may not be one originally supplied. Oops. Comment on attachment 271128 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271128&action=review > Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:133 > + var position = {scrollTop: element.scrollTop, scrollLeft: element.scrollLeft}; Might as well make this "let" too > Source/WebInspectorUI/UserInterface/Views/ContentView.js:44 > + static existingContentViewForRepresentedObject(representedObject) NIT: Shouldn't this go under a "// Static" section? > Source/WebInspectorUI/UserInterface/Views/ContentView.js:47 > + return finalRepresentedObject[WebInspector.ContentView.ContentViewForRepresentedObjectSymbol]; It seems like everywhere you call this function asserts that the returned object is truthy (has value). Maybe move some/all of the asserts above this line and say something along the lines of "Represented Object should have content view" if it is falsy. > Source/WebInspectorUI/UserInterface/Views/ContentView.js:50 > + static closedContentViewForRepresentedObject(representedObject) Should this be "closeContentViewForRepresentedObject" instead of a past-tense version? > Source/WebInspectorUI/UserInterface/Views/ContentView.js:82 > + if (representedObject.sourceCode instanceof WebInspector.Resource) NIT: Are you not combining the "else if" condition with this because it's too long, or is there a different reason? Since they both return the same thing, it looks a bit weird... > Source/WebInspectorUI/UserInterface/Views/ContentView.js:91 > + static _createFromRepresentedObject(representedObject, extraArguments) NIT: Instead of making this a "private" static function, could you just place it inside another function in createFromRepresentedObject? I personally think that having a "public" and "private" function with almost the same name is a bit confusing. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:70 > + var contentView = WebInspector.ContentView.existingContentViewForRepresentedObject(representedObject); NIT: let instead of var > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:407 > + console.assert(contentView.parentContainer !== this); In the case that this is true, I think you should early return to prevent a bunch of extra work from happening. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:414 > + contentView._parentContainer = this; Shouldn't we make _parentContainer into a setter on ContentView? > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486 > + if (otherInterestedContentViews[0] !== this) So, if I am following this right, I think the reason why you're using [0] here is because you're only interested in passing the contentView to the most recent interested view if one exists and is not the current one. If so, could you add a brief statement explaining something to that effect (about the [0]). If not, definitely add a comment. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:519 > + // If this was a tombstone, it should already have been hidden. Should you add an assert here then? Comment on attachment 271128 [details] [PATCH] Proposed Fix Attachment 271128 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/817620 New failing tests: http/tests/xmlhttprequest/workers/referer.html Created attachment 271134 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 271128 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271128&action=review The approach looks good overall, but would like some questions answered and I want to review this again with fresh eyes this afternoon or weekend. It would be good if you lived on this for a few days to make sure nothing is obviously broken. Maybe you can write up some manual test cases that you did along the way to make sure everything is working right. > Source/WebInspectorUI/ChangeLog:89 > + It is an in replace of a content view. Nit: 'is an in replace' ? > Source/WebInspectorUI/ChangeLog:95 > + Deal with closing backforward entries that are tombstones. Nit: back-forward >> Source/WebInspectorUI/UserInterface/Views/ContentView.js:44 >> + static existingContentViewForRepresentedObject(representedObject) > > NIT: Shouldn't this go under a "// Static" section? I would prefer that our API match the old one: ContentView.contentViewForRepresentedObject(object, existingOnly). With this patch it's split between existingContentViewForRepresentedObject, createFromRepresentedObject, and two more helpers. This makes it harder to follow, in my opinion. > Source/WebInspectorUI/UserInterface/Views/ContentView.js:68 > + static _finalRepresentedObjectForRepresentedObject(representedObject) The '_final' terminology is confusing to me. Can we call it 'resolved' or 'displayable' or something like that? When reading it as 'finalRepresentedObject' i had the impression that it was being mutated or something. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:-70 > - // Iterate over all the known content views for the representedObject (if any) and find one that doesn't If we maintain the same API above, then this code will call the same method twice if !onlyExisting. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:89 > representedObject = contentView.representedObject; The result of this assignment is not used. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:112 > + // not owned by us, it may need to be transferred to us. s/us/this container/ > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:405 > + _transferContentView(contentView) From reading the call sites its not always clear whether this method transfer the content view to or from |this|. Can we hint at that with the name? Maybe, becomeParentOfContentView or takeContentView. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:420 > + _placeTombstonesForContentView(contentView) s/place/add/ > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:431 > + for (let entry of this._backForwardList) { This sort of assumes that backForwardList won't be mutated by _hideEntry(). This may be true now, but taking a copy for iterating may be safer. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:447 > + _reviveTombstonesForContentView(contentView) s/revive/clear/ > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:456 > + for (let entry of this._backForwardList) { Ditto. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:465 > + _disassociateFromContentView(contentView, tombstone) What is the intended type of the 'tombstone' argument? It's used as a boolean, but it's not named like one. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:472 > + const onlyFirst = true; Thanks for using a named argument! But, could there be multiple tombstones for the same content view (ie., when switching between two content views several times, then another container transfers it away)? > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:484 > + let otherInterestedContentViews = contentView.__tombstoneContentViewContainers; This variable's name doesn't match its type. It's a content view container. Also the 'interested' terminology is weird, how about: let associatedContainers = ... >> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486 >> + if (otherInterestedContentViews[0] !== this) > > So, if I am following this right, I think the reason why you're using [0] here is because you're only interested in passing the contentView to the most recent interested view if one exists and is not the current one. If so, could you add a brief statement explaining something to that effect (about the [0]). If not, definitely add a comment. There seem to be two cases here when dissociating the content view: - the container is the parent, or - the container has a tombstone In the second case, this code would transfer the content view to some other container. But I would have expected |this| to never be an interested container at this point since we removed it from the tombstone list in the branch above (modulo my comment). So why is this if check necessary? Comment on attachment 271128 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271128&action=review >> Source/WebInspectorUI/UserInterface/Views/ContentView.js:68 >> + static _finalRepresentedObjectForRepresentedObject(representedObject) > > The '_final' terminology is confusing to me. Can we call it 'resolved' or 'displayable' or something like that? When reading it as 'finalRepresentedObject' i had the impression that it was being mutated or something. I like resolved. > Source/WebInspectorUI/UserInterface/Views/ContentView.js:84 > + else if (representedObject.sourceCode instanceof WebInspector.Script) No else. > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:423 > + console.assert(!contentView.__tombstoneContentViewContainers || !contentView.__tombstoneContentViewContainers.includes(this)); Why not a symbol? Comment on attachment 271128 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271128&action=review >> Source/WebInspectorUI/UserInterface/Views/ContentView.js:50 >> + static closedContentViewForRepresentedObject(representedObject) > > Should this be "closeContentViewForRepresentedObject" instead of a past-tense version? I prefer the past tense here, because we call this _after_ we call closed() so that the cached ContentView can be removed. I would have just put this in the base classes' ContentView.prototype.closed but it seems some superclasses do not call super.closed() and I didn't want to clean up all the ContentViews just yet. >> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:414 >> + contentView._parentContainer = this; > > Shouldn't we make _parentContainer into a setter on ContentView? This is a rare oddity in our code. I suppose we could, but the idea is ContentViewContainer is the only one to set it, so we avoided adding a public setter that someone else might use. Weird, I know. >> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:431 >> + for (let entry of this._backForwardList) { > > This sort of assumes that backForwardList won't be mutated by _hideEntry(). This may be true now, but taking a copy for iterating may be safer. This may be too paranoid. I don't think this will ever be the case, but I'll add something to the ChangeLog. >>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486 >>> + if (otherInterestedContentViews[0] !== this) >> >> So, if I am following this right, I think the reason why you're using [0] here is because you're only interested in passing the contentView to the most recent interested view if one exists and is not the current one. If so, could you add a brief statement explaining something to that effect (about the [0]). If not, definitely add a comment. > > There seem to be two cases here when dissociating the content view: > > - the container is the parent, or > - the container has a tombstone > > In the second case, this code would transfer the content view to some other container. But I would have expected |this| to never be an interested container at this point since we removed it from the tombstone list in the branch above (modulo my comment). So why is this if check necessary? In this case "this" is the owning content view container and should not be in the list, so the if check can be dropped. I was using [0] not because it would be the most recent, I was just passing it on to any other interested content view container. I'll improve the comment. >> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:519 >> + // If this was a tombstone, it should already have been hidden. > > Should you add an assert here then? This was explained briefly in the ChangeLog. Closing content views will call hide, and we stick this bail here instead of in closing methods to simplify the logic in closing. We can't necessarily assert that if this is a tombstone the view is hidden, because whoever else is owning the content view might be showing it! We just have to trust that when we made this a tombstone that we hid it if it was the visible content view. Which is the case in placeTombstones. Created attachment 271201 [details]
[PATCH] Proposed Fix
Addressed review comments.
Comment on attachment 271201 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271201&action=review > Source/WebInspectorUI/UserInterface/Views/ContentView.js:178 > + if (representedObject instanceof WebInspector.Breakpoint) { > + if (representedObject.sourceCodeLocation) Could be one line. (In reply to comment #13) > Comment on attachment 271201 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271201&action=review > > > Source/WebInspectorUI/UserInterface/Views/ContentView.js:178 > > + if (representedObject instanceof WebInspector.Breakpoint) { > > + if (representedObject.sourceCodeLocation) > > Could be one line. Its matching the normal create and isViewable style, so I left it Comment on attachment 271201 [details] [PATCH] Proposed Fix Clearing flags on attachment: 271201 Committed r196508: <http://trac.webkit.org/changeset/196508> All reviewed patches have been landed. Closing bug. |