Bug 41858 - Web Inspector: hide AppCache until implemented.
Summary: Web Inspector: hide AppCache until implemented.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on: 41965
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-08 08:07 PDT by Pavel Feldman
Modified: 2010-07-16 08:34 PDT (History)
12 users (show)

See Also:


Attachments
[PATCH] Proposed change. (2.86 KB, patch)
2010-07-08 08:10 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Take 2. Disabled in Chromium, enabled in WebKit. (4.06 KB, patch)
2010-07-16 08:25 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-07-08 08:07:54 PDT
Patch to follow.
Comment 1 Pavel Feldman 2010-07-08 08:10:08 PDT
Created attachment 60884 [details]
[PATCH] Proposed change.
Comment 2 Yury Semikhatsky 2010-07-08 08:16:45 PDT
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.
Comment 3 Pavel Feldman 2010-07-08 08:24:13 PDT
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.
Comment 4 WebKit Commit Bot 2010-07-09 06:45:59 PDT
Comment on attachment 60884 [details]
[PATCH] Proposed change.

Clearing flags on attachment: 60884

Committed r62946: <http://trac.webkit.org/changeset/62946>
Comment 5 WebKit Commit Bot 2010-07-09 06:46:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 WebKit Review Bot 2010-07-09 07:19:41 PDT
http://trac.webkit.org/changeset/62946 might have broken SnowLeopard Intel Release (Tests)
Comment 7 Timothy Hatcher 2010-07-09 09:46:19 PDT
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.
Comment 8 Joseph Pecoraro 2010-07-09 09:49:42 PDT
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).
Comment 9 Pavel Feldman 2010-07-09 09:54:31 PDT
(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.
Comment 10 Joseph Pecoraro 2010-07-09 09:59:20 PDT
(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?
Comment 11 Pavel Feldman 2010-07-09 10:02:34 PDT
(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.
Comment 12 Timothy Hatcher 2010-07-09 10:05:26 PDT
(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.
Comment 13 Timothy Hatcher 2010-07-09 10:08:41 PDT
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.
Comment 14 Pavel Feldman 2010-07-09 10:11:12 PDT
(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.
Comment 15 Timothy Hatcher 2010-07-09 10:16:29 PDT
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.
Comment 16 Timothy Hatcher 2010-07-09 10:17:16 PDT
Yes a preference flag is fine, if it is true in TOT WebKit for Safari.
Comment 17 Pavel Feldman 2010-07-16 08:25:54 PDT
Created attachment 61807 [details]
[PATCH] Take 2. Disabled in Chromium, enabled in WebKit.
Comment 18 Pavel Feldman 2010-07-16 08:34:09 PDT
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