[Webkit-unassigned] [Bug 16440] ER: file replacement support

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=16440


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #19116|review?                     |review-
               Flag|                            |




------- Comment #10 from darin at apple.com  2008-03-13 10:07 PDT -------
(From update of attachment 19116)
Thanks for contributing to WebKit!

This patch has a lot of problems.

+            //Bug 16440: ER: file replacement support - for .css files

We put spaces after "//" and we don't tag code with the bug number of the bug
used to add the code.

+            if( document()->frame()->isReplaceCFPropListReady &&
document()->frame()->isURLPresentinDic(keyURL)) {

No space after the parenthesis.

+                               if( m_doc->frame()->isReplaceCFPropListReady &&
m_doc->frame()->isURLPresentinDic(keyURL)) {

Formatting done here with tags.

+        String newstr = m_frame->getValueURLfromDic(keyURL);
+        KURL url(newstr.utf8().data());

This is the wrong way to convert a string to a KURL. There's no reason to
convert the string to UTF-8 just to construct the URL.

+       
m_frame->setJSStatusBarText(page->inspectorController()->setJSStatusBarTextForLoalload());

What's a "loalload"? It's not appropriate to set status bar text directly.
Putting status bar text in at this time would be a policy decision for the web
browser, not for WebKit itself. The setJSStatusBarText function is specifically
to pass status bar text from JavaScript to the browser, and must not be used to
add other status messages.

+
+     //Hack to replace urls -start
+     isReplaceCFPropListReady=FALSE;
+     storeReplaceURL();

Unclear comment.

+    bool isURLPresentinDic(String keyURL);
+    String getValueURLfromDic( String keyURL);
+    void storeReplaceURL();
+    bool isReplaceCFPropListReady;
+    String getPlistURL();
+    String getCurrentUserHomeDir();
+    bool isFileTobeReplaced( JSStringRef replaceURL );
+    bool isFileExists(JSStringRef file);
+    
+    void addToPlist(JSStringRef urlToAdd);
+    String getValueForKey( String url );
+    bool isKeyPresent( JSStringRef key);
+    void deleteAKeyFromPlist(JSStringRef key);
+    void saveScriptsLocally( JSStringRef url, JSStringRef source );

Lots wrong here:

    1) We are not adding new functions and data to Frame at this time.
Functions need to go elsewhere, on one of the appropriate Frame sub-objects. I
don't understand this feature you're adding, so it's hard to know where it
would go, but if it's about loading it probably belongs on FrameLoader.
    2) These function names have abbreviations and style inconsistent with the
rest of WebKit. For example, we don't use the word "Dic" and I have no idea
what that refers to.
    3) Data members should be private, not public.
    4) JavaScript types like JSStringRef should not be used in general loader
code.
    5) We don't use "get" in the titles of functions that return values.
    6) String parameters should be "const String&" rather than just string.
    7) getValueForKey, for example, is way too generic a title for a function
on Frame. What kind of value? What kind of key? A Frame is not a dictionary.
    8) The references to "plist" are inappropriate for cross-platform code.
That's a Macintosh-specific concept.

All this new code for the development tools probably doesn't belong in the core
WebKit, but in some other class on the side. You should talk with some of the
folks working on the inspector and get their suggestions about where to put
this code.

My first thought is that editing of a local copy of a file should probably be
lower level down at the loader/cache (classes like Loader and CachedResource)
rather up in the frame and frame loader.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.