Bug 41858 – Web Inspector: hide AppCache until implemented.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 60884 [details]
[PATCH] Proposed change.

[Yury Semikhatsky]

Comment on attachment 60884 [details]
[PATCH] Proposed change.

WebCore/inspector/front-end/StoragePanel.js:52
 +      // TODO: uncomment when AppCache is implemented.
Please put bug number after the TODO.

[Pavel Feldman]

Comment on attachment 60884 [details]
[PATCH] Proposed change.

(In reply to comment #2)
> (From update of attachment 60884 [details])
> WebCore/inspector/front-end/StoragePanel.js:52
>  +      // TODO: uncomment when AppCache is implemented.
> Please put bug number after the TODO.

There is no particular bug yet. But this is a live topic, so I expect this todo to vanish shortly.

[WebKit Commit Bot]

Comment on attachment 60884 [details]
[PATCH] Proposed change.

Clearing flags on attachment: 60884

Committed r62946: <http://trac.webkit.org/changeset/62946>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62946 might have broken SnowLeopard Intel Release (Tests)

[Timothy Hatcher]

FWIW, it is working enough on Safari/WebKit to having it enabled. Please change it so it stays enabled in WebKit, and Chrome disables it.

[Joseph Pecoraro]

Checking a global variable or preference would work nicer. Something like
in inspector.js:

  // TODO: eventually enable application cache
  WebInspector.applicationCacheEnabled = false;

And then instead of commenting out the code, you just wrap it in a block around
this variable. This means that when we want to enable the feature locally, adding
new features and enhancements we only have to change this variable to true and
revert it to false when making a patch. With this patch we have to uncomment a lot
of code in different places (and comment it back when making patches). When
finally enabling the feature we would just delete the variable and its checks (easily
searchable).

[Pavel Feldman]

(In reply to comment #7)
> FWIW, it is working enough on Safari/WebKit to having it enabled. Please change it so it stays enabled in WebKit, and Chrome disables it.

Timothy, I'd appreciate if we used the same process as while implementing Timeline. We make it work and then agree on enabling it. Michael was pointing out the problems that make the feature look incomplete.

Like 
- start the app
- go to Storage
- click on appcache icon
- click on localStore icon

appcache page is not hidden. So the thing is really broken to be enabled.

[Joseph Pecoraro]

(In reply to comment #9)
> Like 
> - start the app
> - go to Storage
> - click on appcache icon
> - click on localStore icon
> 
> appcache page is not hidden. So the thing is really broken to be enabled.

I have never encountered this problem. It works in Safari. If I saw this I would
certainly have fixed it. Does an error get thrown in the inspector's inspector
for Chrome? Maybe a check I missed if there was no data yet?

[Pavel Feldman]

(In reply to comment #10)
> (In reply to comment #9)
> > Like 
> > - start the app
> > - go to Storage
> > - click on appcache icon
> > - click on localStore icon
> > 
> > appcache page is not hidden. So the thing is really broken to be enabled.
> 
> I have never encountered this problem. It works in Safari. If I saw this I would
> certainly have fixed it. Does an error get thrown in the inspector's inspector
> for Chrome? Maybe a check I missed if there was no data yet?

I tried it in Chromium, did not try in Safari. That is not the main point though.

[Timothy Hatcher]

(In reply to comment #9)
> (In reply to comment #7)
> > FWIW, it is working enough on Safari/WebKit to having it enabled. Please change it so it stays enabled in WebKit, and Chrome disables it.
> 
> Timothy, I'd appreciate if we used the same process as while implementing Timeline. We make it work and then agree on enabling it. Michael was pointing out the problems that make the feature look incomplete.
> 
> Like 
> - start the app
> - go to Storage
> - click on appcache icon
> - click on localStore icon
> 
> appcache page is not hidden. So the thing is really broken to be enabled.

Was that in WebKit/Safari? I agree the feature should be hidden in Chrome is it isn't ready. Timeline panel was different, sicne it was a large new feature and could easily be hidden by removing the panel. This was multiple lines of code that got commented out and prevents easy developemt as Joe pointed out.

I think this patch needs to be reverted or changed to check a global variable. And preferibly enabled in WebKit/Safari and disabled in Chrome until it is ready.

[Timothy Hatcher]

Also we don't normally land commented out code. We either remove it or hide it being a flag. So this patch should have not landed.

[Pavel Feldman]

(In reply to comment #13)
> Also we don't normally land commented out code. We either remove it or hide it being a flag. So this patch should have not landed.

k, I am happy to hide it behind the preference flag. Will that be fine by you?

I am actually a bit surprised seeing the reaction. My perception was that I am hiding it for you. As I understand this is change was copying Kavita's code, reworking it and landing it enabled without her permission / talking to her.

[Timothy Hatcher]

I guess the problem was on our end, we should have made sure it was disabled in Chrome when it got added to the UI. Similar to how V8 only features are done.

[Timothy Hatcher]

Yes a preference flag is fine, if it is true in TOT WebKit for Safari.

[Pavel Feldman]

Created attachment 61807 [details]
[PATCH] Take 2. Disabled in Chromium, enabled in WebKit.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/Settings.js
	M	WebCore/inspector/front-end/StoragePanel.js
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/js/DevTools.js
Committed r63549