Bug 81414

Summary: Web Inspector: newly added CSS rules shouldn’t rewrite HTML content in the resources panel
Product: WebKit Reporter: Nikita Vasilyev <me>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Status: RESOLVED FIXED    
Severity: Normal CC: aneganov, apavlov, bweinstein, caseq, joepeck, keishi, lajlev, loislo, paulirish, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch vsevik: review+

Description Nikita Vasilyev 2012-03-16 15:08:44 PDT
http://www.screenr.com/jvN8

Adding CSS rule rewrites HTML content in the resources panel. Wouldn’t it be better put it into the new item under "Stylesheets" category?
Comment 1 Alexander Pavlov (apavlov) 2012-03-20 05:05:02 PDT
Created attachment 132804 [details]
Patch
Comment 2 Vsevolod Vlasov 2012-03-20 05:07:50 PDT
Comment on attachment 132804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132804&action=review

> Source/WebCore/ChangeLog:3
> +        Web Inspector: newly added CSS rules shouldnât rewrite HTML content in the resources panel

shouldn't
Comment 3 Alexander Pavlov (apavlov) 2012-03-20 05:17:41 PDT
Committed r111384: <http://trac.webkit.org/changeset/111384>
Comment 4 Nikita Vasilyev 2012-03-20 09:41:40 PDT
Now newly added CSS rules don’t change HTML content, which is good, but how do I see them in resources panel? I might want to save them. I originally suggested to add 'new item under "Stylesheets" category', but you haven’t implemented it and marked the issue as RESOLVED FIXED without any comments.
Comment 5 Alexander Pavlov (apavlov) 2012-03-20 09:56:34 PDT
The landed patch fixes the erroneous behavior stated in the bug summary. The behavior you suggest is not that easy to implement UI-wise, since a page can contain an arbitrary number of inline stylesheets, all of which should be displayed consistently in the Resources panel. Also, it is not wise to postpone a fix for such a conspicuous bug until a complicated solution is in place.

That said, you can track the progress of bug 62294 which attempts to implement what you want to see.
Comment 6 Nikita Vasilyev 2012-03-20 13:56:37 PDT
Chrome fires the following code on adding new CSS rules. Does your patch change that?

chrome.devtools.inspectedWindow.onResourceContentCommitted.addListener(function(event) {
  event.type // 'document'
  event.url // URL of HTML page
});
Comment 7 Alexander Pavlov (apavlov) 2012-03-20 14:38:01 PDT
(In reply to comment #6)
> Chrome fires the following code on adding new CSS rules. Does your patch change that?
> 
> chrome.devtools.inspectedWindow.onResourceContentCommitted.addListener(function(event) {
>   event.type // 'document'
>   event.url // URL of HTML page
> });

This extension event is (indirectly) dispatched from WebInspector.Resource.prototype.addRevision(), which is no longer called when a stylesheet with a URL corresponding to a non-stylesheet resource is changed.

Andrey, is the first part of this statement correct? AFAIU, your ExtensionServer subscribes to the WebInspector.ResourceTreeModel.EventTypes.ResourceContentCommitted event dispatched from WebInspector.Resource.prototype.addRevision().
Comment 8 Nikita Vasilyev 2012-03-21 18:06:48 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Chrome fires the following code on adding new CSS rules. Does your patch change that?
> > 
> > chrome.devtools.inspectedWindow.onResourceContentCommitted.addListener(function(event) {
> >   event.type // 'document'
> >   event.url // URL of HTML page
> > });

I just checked, Chrome 19.0.1076.0 canary does not fire it anymore. It makes saving newly added CSS rules impossible, please roll it back!
Comment 9 Alexander Pavlov (apavlov) 2012-03-22 02:54:30 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Chrome fires the following code on adding new CSS rules. Does your patch change that?
> > > 
> > > chrome.devtools.inspectedWindow.onResourceContentCommitted.addListener(function(event) {
> > >   event.type // 'document'
> > >   event.url // URL of HTML page
> > > });
> 
> I just checked, Chrome 19.0.1076.0 canary does not fire it anymore. It makes saving newly added CSS rules impossible, please roll it back!

So, you are fine with rewriting the main document HTML content the way it was before the fix (the onResourceContentCommitted event you describe means just that)? What do you expect to see in the Resources panel when a new rule is added?
Comment 10 Nikita Vasilyev 2012-03-22 03:09:38 PDT
https://groups.google.com/forum/#!topic/google-chrome-developer-tools/-c37nQPrcv4

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Chrome fires the following code on adding new CSS rules. Does your patch change that?
> > > > 
> > > > chrome.devtools.inspectedWindow.onResourceContentCommitted.addListener(function(event) {
> > > >   event.type // 'document'
> > > >   event.url // URL of HTML page
> > > > });
> > 
> > I just checked, Chrome 19.0.1076.0 canary does not fire it anymore. It makes saving newly added CSS rules impossible, please roll it back!
> 
> So, you are fine with rewriting the main document HTML content the way it was before the fix (the onResourceContentCommitted event you describe means just that)? What do you expect to see in the Resources panel when a new rule is added?

It allowed me to catch newly added CSS rules, now it doesn’t. It does have issues https://groups.google.com/forum/#!topic/google-chrome-developer-tools/-c37nQPrcv4 but it’s better to keep it as until better substitute is found.
Comment 11 Michael Lajlev 2012-04-11 01:20:43 PDT
Any idea when it gonna be fixed?
Comment 12 Artiom Neganov 2012-05-23 01:13:10 PDT
I see it is "FIXED". Well, the fixness means avioding responsibility, I suspect. 
Do you every consider consequences?

This change is a disaster. It is blocking for all web devs moved their development partially or fully from their off-line apps to DevTools. 

Now watching this demolishing change I feel dismayed, and the only option now is to rollback to 18th.

Thank you!
Comment 13 Pavel Feldman 2012-05-23 02:09:37 PDT
(In reply to comment #12)
> I see it is "FIXED". Well, the fixness means avioding responsibility, I suspect. 
> Do you every consider consequences?
> 
> This change is a disaster. It is blocking for all web devs moved their development partially or fully from their off-line apps to DevTools. 
> 
> Now watching this demolishing change I feel dismayed, and the only option now is to rollback to 18th.
> 
> Thank you!

Nikita, do you think you can address it on your end?
Comment 14 Nikita Vasilyev 2012-05-23 02:34:52 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > I see it is "FIXED". Well, the fixness means avioding responsibility, I suspect. 
> > Do you every consider consequences?
> > 
> > This change is a disaster. It is blocking for all web devs moved their development partially or fully from their off-line apps to DevTools. 
> > 
> > Now watching this demolishing change I feel dismayed, and the only option now is to rollback to 18th.
> > 
> > Thank you!
> 
> Nikita, do you think you can address it on your end?

No. onResourceContentCommitted doesn’t fire. It used to do.
Comment 15 Pavel Feldman 2012-05-23 02:42:51 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > I see it is "FIXED". Well, the fixness means avioding responsibility, I suspect. 
> > > Do you every consider consequences?
> > > 
> > > This change is a disaster. It is blocking for all web devs moved their development partially or fully from their off-line apps to DevTools. 
> > > 
> > > Now watching this demolishing change I feel dismayed, and the only option now is to rollback to 18th.
> > > 
> > > Thank you!
> > 
> > Nikita, do you think you can address it on your end?
> 
> No. onResourceContentCommitted doesn’t fire. It used to do.

So we are talking about (+) "New Style Rule" action. Users that are adding rules into the existing stylesheets via editing them are not affected?
Comment 16 Nikita Vasilyev 2012-05-23 04:14:50 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > I see it is "FIXED". Well, the fixness means avioding responsibility, I suspect. 
> > > > Do you every consider consequences?
> > > > 
> > > > This change is a disaster. It is blocking for all web devs moved their development partially or fully from their off-line apps to DevTools. 
> > > > 
> > > > Now watching this demolishing change I feel dismayed, and the only option now is to rollback to 18th.
> > > > 
> > > > Thank you!
> > > 
> > > Nikita, do you think you can address it on your end?
> > 
> > No. onResourceContentCommitted doesn’t fire. It used to do.
> 
> So we are talking about (+) "New Style Rule" action. Users that are adding rules into the existing stylesheets via editing them are not affected?

Correct.
Comment 17 Pavel Feldman 2012-05-24 10:12:33 PDT
I landed http://trac.webkit.org/changeset/118367 that introduces the concept of the virtual resource for the inspector stylesheet. Could you check if your extension can be tweaked to support it?
Comment 18 Pavel Feldman 2012-05-24 10:12:54 PDT
(In reply to comment #17)
> I landed http://trac.webkit.org/changeset/118367 that introduces the concept of the virtual resource for the inspector stylesheet. Could you check if your extension can be tweaked to support it?

(That was for Nikita)
Comment 19 Nikita Vasilyev 2012-05-25 06:50:23 PDT
(In reply to comment #17)
> I landed http://trac.webkit.org/changeset/118367 that introduces the concept of the virtual resource for the inspector stylesheet. Could you check if your extension can be tweaked to support it?

It works again https://github.com/NV/chrome-devtools-autosave/commit/627534b32edc87619e448ab882e877726663f714.
Thanks!