http://bugs.webkit.org/show_bug.cgi?id=16440
------- Comment #11 from sajesh.ramachandran at apple.com 2008-03-18 15:57 PDT -------
(In reply to comment #10)
> (From update of attachment 19116 [edit])
> 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.
This is implemented
>
> + if( document()->frame()->isReplaceCFPropListReady &&
> document()->frame()->isURLPresentinDic(keyURL)) {
>
> No space after the parenthesis.
>
This is implemented
> + if( m_doc->frame()->isReplaceCFPropListReady &&
> m_doc->frame()->isURLPresentinDic(keyURL)) {
>
> Formatting done here with tags.
>
This is implemented
> + 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.
>
This is implemented
> +
> 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.
>
Changing the status bar text is removed.
> +
> + //Hack to replace urls -start
> + isReplaceCFPropListReady=FALSE;
> + storeReplaceURL();
>
> Unclear comment.
This is taken care and comments are udpated
>
> + 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.
The functionality is to save script files from Inspector window to local file
system and to relaod this local file while reloading the page. This is done for
debugging script files locally. The loading part is done in the FrameLoader
class . The saving of the files is done on FrameMac.mm as it requires access to
the frame resource to get the source.
> 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.
Taken care of this, changed the names to suit the fucntionality
> 3) Data members should be private, not public.
This has been taken care.
> 4) JavaScript types like JSStringRef should not be used in general loader
> code.
This has been taken care.
> 5) We don't use "get" in the titles of functions that return values.
This has been taken care.
> 6) String parameters should be "const String&" rather than just string.
This has been taken care
> 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.
This has been taken care
> 8) The references to "plist" are inappropriate for cross-platform code.
> That's a Macintosh-specific concept.
This functionality is now supported in Mac OS only so its using plist and
CFDictionry. This is used in Mac specific classes.
>
> 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.
>
Will discuss with Inspector windows team to get their suggestions and will
incorporate that changes.
> 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.