Created attachment 126741 [details] WIP First level WIP. This has implementation of common base class and replacement setter/getter function used in page/Location.

Attachment 126741 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/html/URLAttributes.cpp:92: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126741 [details] WIP Attachment 126741 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11507761

Comment on attachment 126741 [details] WIP Attachment 126741 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11509808 New failing tests: fast/dom/Window/invalid-protocol.html fast/history/history-back-within-subframe-hash.html http/tests/xmlhttprequest/workers/referer.html fast/forms/submit-change-fragment.html http/tests/loading/location-hash-reload-cycle.html fast/history/history-back-forward-within-subframe-hash.html fast/history/same-document-iframes-changing-fragment.html fast/loader/about-blank-hash-change.html fast/history/same-document-iframes-changing-pushstate.html fast/loader/hashchange-event.html http/tests/inspector/change-iframe-src.html http/tests/history/popstate-fires-with-pending-requests.html fast/dom/location-new-window-no-crash.html http/tests/inspector/inspect-iframe-from-different-domain.html fast/dom/location-hash.html fast/loader/stateobjects/pushstate-with-fragment-urls-and-hashchange.html fast/history/location-replace-hash.html

What does this buy us? From the patch, it doesn't appear that a lot of code sharing is achieved, nor does the size of code go down. These attributes are slightly different anyway.

(In reply to comment #5) > What does this buy us? From the patch, it doesn't appear that a lot of code sharing is achieved, nor does the size of code go down. > > These attributes are slightly different anyway. Based on the Adam's suggestion, we should be creating a common base class to accommodate all these 8 attributes's setter & getter functions. Current patch is just first cut implementation of base class and migrating Location's class functions to this class. The purpose of uploading this patch was to get comments if I am going on right direction. Will be uploading one more WIP which will have HTMLAnchorElement implementation.

We'll need to see how much this helps. KURL is pretty much the class that implements this.

True, KURL has its own implementation. We might fix the KURL ascii issues and make it complete so that all these Location/HTMLAnchorElement/DOMURL can use it without needing any more modification on url.

You are using KURL's implementation already. What I'm saying is that even when there is a lot of code to share, adding a common base class is the wrong solution. Inheritance is not a method of code sharing. In this particular case, it seems that there is no issue to solve in the first place.

(In reply to comment #9) > You are using KURL's implementation already. > > What I'm saying is that even when there is a lot of code to share, adding a common base class is the wrong solution. Inheritance is not a method of code sharing. > > In this particular case, it seems that there is no issue to solve in the first place. From your comments what I get is I should implement the URL decomposition attributes as it is without using base class as this is not much code saving mechanism to make a base class. If so, I will close this bug and will update the patch in https://bugs.webkit.org/show_bug.cgi?id=76816.

I think that this would be more like what Adam was referring to, as well. As you've seen, KURL already has code for most of what this API exposes.

(In reply to comment #11) > I think that this would be more like what Adam was referring to, as well. > > As you've seen, KURL already has code for most of what this API exposes. Yes, KURL already has almost complete APIs for the same. And all these setter/getter functions have little or No modification over these. So, I will close this bug and will update the blocking issue with updated patch.

Closing the bugs as per discussion. Will reopen if in future there would be

Closing the bugs as per discussion. Will reopen if in future there would be need to code re-factoring.