| Summary: | [EFL] Web Inspector: Move to new web inspector | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Marcelo Morais <m.morais> | ||||||||||||||||||||||||
| Component: | Web Inspector | Assignee: | Marcelo Morais <m.morais> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | andre.vl, buildbot, cdumez, commit-queue, graouts, gyuyoung.kim, gyuyoung.kim, joepeck, ossy, rakuco, rniwa, rtakacs, seokju, timothy, tonikitoo | ||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||
| Bug Blocks: | 118676 | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Marcelo Morais
2013-08-07 15:06:58 PDT
Created attachment 209113 [details]
Screenshot from first version
After hooking up some of the bits needed, I am able to launch a functional version of the new Inspector in EFL's MiniBrowser (see attachedment #209113). As one might notice, some of the SVG icons at the top are not properly displayed. Have you guys faced similar issues or have an idea on where to start debugging it from? Heh, that is pretty wild. For starters, you may want to give #toolbar a background gradient / color instead of white. Probably somewhat like the old inspector did this. I think it checked InspectorFrontendHost.platform and added a style class to the body, and used that in CSS selectors for setting a toolbar background. As for why the embossing doesn't work, I don't know. Try bumping _imageStorageFormatVersion in ImageUtilities.js to see if we're still reusing the generated images from before switching to SVG images (generated images are stored in a WebSQL Database). If that doesn't work, then you will need to dig into ImageUtilities generateEmbossedImages. Created attachment 209677 [details]
New screenshot
Hello,
After a week working with Andre Loureiro in this bug we didn't find any reason to WebInspector looks that weird as shown in the first picture.
We debug ImageUtilities, changed some css files and tried another things but the inspector wasn't getting any better.
Today i did a rebase with webkit trunk and the inspector looks good without any changes, so something wasn't good in the hash that we were using but is fixed now.
I think that the presentation is good now, but still have some work to do and we have to test it completly.
I hope we have the patch later this week.
Ahh, this may have been when only some of the images were converted to SVGs but not all of them where yet. Excellent, looking forward to a patch! Created attachment 210111 [details]
Patch
Hi,
Here is the patch.
The new inspector is working fine with MiniBrowser and with EWebLauncher.
The known issues are:
1 - Like the old inspector, the dock controls are not working.
2 - Context menu are not working, MiniBrowser and EWebLauncher needs some work to enable it, in the old inspector also have problems like that, e.g open the menu on elements tab using the mouse's right click.
3 - The frame's combo box in content browser area is not working.
But this are minor issues and do not affect the main working of the inspector.
Maybe a good idea is to open bugs to improve these browsers and fix these issues.
We don't know about layout tests for this new WebInspector, the old ones aren't skipped in TestExpectations.
Are there any plans to these new Layout Tests for the new inspector?
Comment on attachment 210111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210111&action=review Otherwise looks good to me. > Source/WebKit2/ChangeLog:2 > +2013-08-28 Marcelo Morais <m.morais@samsung.com> > +2013-08-28 Andre Loureiro <andre.vl@samsung.com> I don't think this is typical. Usually you mention other people in the description and only supply one ChangeLog header. Otherwise this might confuse some scripts we have. (In reply to comment #7) > Created an attachment (id=210111) [details] > The new inspector is working fine with MiniBrowser and with EWebLauncher. Excellent! > The known issues are: > > 1 - Like the old inspector, the dock controls are not working. > 2 - Context menu are not working, MiniBrowser and EWebLauncher needs some work to enable it, in the old inspector also have problems like that, e.g open the menu on elements tab using the mouse's right click. > 3 - The frame's combo box in content browser area is not working. > > But this are minor issues and do not affect the main working of the inspector. How about fonts and keyboard shortcuts? I suspect the font situation could be improved and keyboard shortcuts should likely be using Ctrl instead of Meta right now (most of the KeyboardShortcuts right now are tuned to OS X with ⌘ (Command) key. > Maybe a good idea is to open bugs to improve these browsers and fix these issues. Yes, that is the ideal path to getting these fixed. Quick link to create a Web Inspector bug is: <http://webkit.org/new-inspector-bug> > We don't know about layout tests for this new WebInspector, the old ones aren't skipped in TestExpectations. > > Are there any plans to these new Layout Tests for the new inspector? Yep you're correct. Instead of testing the UserInterface we're putting I've been focus on testing the protocol (LayoutTest/inspector-protocol). The existing LayoutTests/inspector tests are pretty tightly coupled with the old inspector frontend. That makes changing the frontend more difficult and time consuming because you also have to update the tests that relied on private functions / interfaces in the frontend. Created attachment 210153 [details] Fixed Changelog Thanks for the comments :) Fixed Changelog now. About Joseph's comments, we didn't touch in fonts and keyboard shortcuts. I talked with Andre Loureiro about that and we saw that patch for enable the new inspector for Windows landed recently (https://bugs.webkit.org/show_bug.cgi?id=120098) didn't touch with this as well, maybe we need create another bug for these general changes, don't you think? (In reply to comment #10) > Created an attachment (id=210153) [details] > … maybe we need create another bug for these general changes, don't you think? Yep, I agree. That was what I was suggesting as well. Comment on attachment 210153 [details] Fixed Changelog View in context: https://bugs.webkit.org/attachment.cgi?id=210153&action=review > Source/PlatformEfl.cmake:-37 > - find_program(UGLIFYJS_EXECUTABLE uglifyjs) > - if (UGLIFYJS_EXECUTABLE AND (NOT ${CMAKE_BUILD_TYPE} STREQUAL "Debug")) > - file(GLOB frontend_js_files "${WEBCORE_DIR}/inspector/front-end/*.js") > - set(all_js_files > - ${frontend_js_files} > - "${WEBCORE_DIR}/English.lproj/localizedStrings.js" > - "${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js" > - ) > - > - foreach (js_file ${all_js_files}) > - get_filename_component(filename ${js_file} NAME) > - install(CODE > - "execute_process( > - COMMAND ${UGLIFYJS_EXECUTABLE} --overwrite ${filename} > - WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/${WEB_INSPECTOR_DIR})") > - endforeach () > - endif () It looks like this used to combine and minify the JS files for the old frontend. You may want to do something like this for the new frontend. The code would likely be very similar. (In reply to comment #12) > (From update of attachment 210153 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210153&action=review > > > Source/PlatformEfl.cmake:-37 > > - find_program(UGLIFYJS_EXECUTABLE uglifyjs) > > - if (UGLIFYJS_EXECUTABLE AND (NOT ${CMAKE_BUILD_TYPE} STREQUAL "Debug")) > > - file(GLOB frontend_js_files "${WEBCORE_DIR}/inspector/front-end/*.js") > > - set(all_js_files > > - ${frontend_js_files} > > - "${WEBCORE_DIR}/English.lproj/localizedStrings.js" > > - "${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js" > > - ) > > - > > - foreach (js_file ${all_js_files}) > > - get_filename_component(filename ${js_file} NAME) > > - install(CODE > > - "execute_process( > > - COMMAND ${UGLIFYJS_EXECUTABLE} --overwrite ${filename} > > - WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/${WEB_INSPECTOR_DIR})") > > - endforeach () > > - endif () > > It looks like this used to combine and minify the JS files for the old frontend. You may want to do something like this for the new frontend. The code would likely be very similar. Do you mean execute WebInspectorUI/Scripts/jsmin.py and cssmin.py? > Do you mean execute WebInspectorUI/Scripts/jsmin.py and cssmin.py?
Yes, either that or sticking with uglifyjs like you have now for minifying the JavaScript.
Created attachment 210160 [details]
Fixed patch
Fixes suggested by Joseph.
Comment on attachment 210160 [details] Fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=210160&action=review > Source/PlatformEfl.cmake:25 > - file(GLOB frontend_js_files "${WEBCORE_DIR}/inspector/front-end/*.js") > + file(GLOB frontend_js_files "${PROJECT_SOURCE_DIR}/Source/WebInspectorUI/UserInterface/*.js") > set(all_js_files > ${frontend_js_files} > - "${WEBCORE_DIR}/English.lproj/localizedStrings.js" > - "${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js" > + "${PROJECT_SOURCE_DIR}/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js" > ) This uglifyjs command below does "--overwrite". So it sounds like you would want to only update the ${WEB_INSPECTOR_DIR} files, not the actual files in Source/WebInspectorUI. Especially if you have already copied the files over. The old code had the same problem, so I'm not sure what this part of code is doing. I would expect to see something like: file(GLOB frontend_js_files "${WEB_INSPECTOR_DIR}/*.js") file(GLOB codemirror_js_files "${WEB_INSPECTOR_DIR}/External/CodeMirror/*.js") set(all_js_files ${frontend_js_files} ${codemirror_js_files} ) Does that make sense? > Source/PlatformEfl.cmake:6 > + COMMAND ${CMAKE_COMMAND} -E copy_directory ${PROJECT_SOURCE_DIR}/Source/WebInspectorUI/UserInterface ${WEB_INSPECTOR_DIR} How about using variable for ${PROJECT_SOURCE_DIR}/Source/WebInspectorUI ? in CMakeLists.txt +set(WEBINSPECTORUI_DIR "${CMAKE_SOURCE_DIR}/Source/WebInspectorUI") > Source/PlatformEfl.cmake:7 > + COMMAND ${CMAKE_COMMAND} -E copy ${PROJECT_SOURCE_DIR}/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js ${WEB_INSPECTOR_DIR} Ditto. > Source/PlatformEfl.cmake:24 > + "${PROJECT_SOURCE_DIR}/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js" Ditto. And you need to add new filetype(SVG) for FILES_MATCHING of install, or remove it. > I would expect to see something like:
>
> file(GLOB frontend_js_files "${WEB_INSPECTOR_DIR}/*.js")
> file(GLOB codemirror_js_files "${WEB_INSPECTOR_DIR}/External/CodeMirror/*.js")
> set(all_js_files
> ${frontend_js_files}
> ${codemirror_js_files}
> )
>
> Does that make sense?
Hi, I'm not sure about it, so I've looked at WebInspectorUI/Scripts and found some python and shell scripts there, so there's one that I saw called copy-user-interface-resources.sh, so I think if we try to execute this one at build time it could resolve all these related issues about minify the .js and .css code, if I am wrong, please clarify these scripts.
Thanks.
Created attachment 210310 [details]
patch
Sorry for the long delay, I was testing the uglifyjs.
In this new patch, i added the variable according to Seokju Kwon suggested.
About the uglifyjs, the code is correct, the code minimization will happen when installing webkit.
You can see the minimized code in /usr/local/share/ewebkit-0/inspector (this is the default installation path).
Comment on attachment 210310 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=210310&action=review > Source/cmake/OptionsEfl.cmake:237 > + set(WEBINSPECTORUI_DIR "${PROJECT_SOURCE_DIR}/Source/WebInspectorUI") Use CMAKE_SOURCE_DIR instead of PROJECT_SOURCE_DIR for consistency? Created attachment 210365 [details]
patch
Hi Seokju, thanks for the review, the patch is fixed with your suggestion now.
Comment on attachment 210365 [details] patch Attachment 210365 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1687331 New failing tests: fast/workers/termination-with-port-messages.html Created attachment 210371 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
BTW, can't we adjust this into EFL WK2 as well ? (In reply to comment #24) > BTW, can't we adjust this into EFL WK2 as well ? Hello Gyuyoung Kim, could you explain better to me? Because this is already working on EFL WK2 using MiniBrowser. Thanks! (In reply to comment #25) > (In reply to comment #24) > > BTW, can't we adjust this into EFL WK2 as well ? > > Hello Gyuyoung Kim, could you explain better to me? > > Because this is already working on EFL WK2 using MiniBrowser. Aha, I see. I didn't check it. > Aha, I see. I didn't check it.
:) Any comments or do you think its good now?
Comment on attachment 210365 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=210365&action=review R- because of wrong ChangeLog. > Source/WebKit2/ChangeLog:11 > + * WebCoreSupport/InspectorClientEfl.cpp: This patch doesn't touch WebCoreSupport/InspectorClientEfl.cpp. > Source/WebKit2/ChangeLog:14 > + (WebKit::WebInspectorProxy::inspectorPageURL): Please mention what is changed here. > Source/WebKit2/ChangeLog:15 > + * Source/PlatformEfl.cmake: I wonder why this file is mentioned in WebKit2/ChangeLog. Don't you generate ChangeLog using Prepare-ChangeLog script ? Created attachment 210461 [details]
Fixed Changelog
Fixed Changelog.
Comment on attachment 210461 [details] Fixed Changelog View in context: https://bugs.webkit.org/attachment.cgi?id=210461&action=review > Source/PlatformEfl.cmake:-9 > - COMMAND ${CMAKE_COMMAND} -E copy ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js ${WEB_INSPECTOR_DIR} Don't you need to install these files as well ? if (UGLIFYJS_EXECUTABLE AND (NOT ${CMAKE_BUILD_TYPE} STREQUAL "Debug")) file(GLOB frontend_js_files "${WEBCORE_DIR}/inspector/front-end/*.js") set(all_js_files ${frontend_js_files} "${WEBCORE_DIR}/English.lproj/localizedStrings.js" "${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js" ) foreach (js_file ${all_js_files}) get_filename_component(filename ${js_file} NAME) install(CODE "execute_process( COMMAND ${UGLIFYJS_EXECUTABLE} --overwrite ${filename} WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/${WEB_INSPECTOR_DIR})") endforeach () endif () (In reply to comment #30) > (From update of attachment 210461 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210461&action=review > > > Source/PlatformEfl.cmake:-9 > > - COMMAND ${CMAKE_COMMAND} -E copy ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js ${WEB_INSPECTOR_DIR} > > Don't you need to install these files as well ? > > if (UGLIFYJS_EXECUTABLE AND (NOT ${CMAKE_BUILD_TYPE} STREQUAL "Debug")) > file(GLOB frontend_js_files "${WEBCORE_DIR}/inspector/front-end/*.js") > set(all_js_files > ${frontend_js_files} > "${WEBCORE_DIR}/English.lproj/localizedStrings.js" > "${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js" > ) > > foreach (js_file ${all_js_files}) > get_filename_component(filename ${js_file} NAME) > install(CODE > "execute_process( > COMMAND ${UGLIFYJS_EXECUTABLE} --overwrite ${filename} > WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/${WEB_INSPECTOR_DIR})") > endforeach () > endif () Use in WebInspectorUI/UserInterface/InspectorBackendCommands.js in New Web Inspector. (In reply to comment #31) > (In reply to comment #30) > > (From update of attachment 210461 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=210461&action=review > > > > > Source/PlatformEfl.cmake:-9 > > > - COMMAND ${CMAKE_COMMAND} -E copy ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js ${WEB_INSPECTOR_DIR} > > > > Don't you need to install these files as well ? > > > > if (UGLIFYJS_EXECUTABLE AND (NOT ${CMAKE_BUILD_TYPE} STREQUAL "Debug")) > > file(GLOB frontend_js_files "${WEBCORE_DIR}/inspector/front-end/*.js") > > set(all_js_files > > ${frontend_js_files} > > "${WEBCORE_DIR}/English.lproj/localizedStrings.js" > > "${DERIVED_SOURCES_WEBCORE_DIR}/InspectorBackendCommands.js" > > ) > > > > foreach (js_file ${all_js_files}) > > get_filename_component(filename ${js_file} NAME) > > install(CODE > > "execute_process( > > COMMAND ${UGLIFYJS_EXECUTABLE} --overwrite ${filename} > > WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/${WEB_INSPECTOR_DIR})") > > endforeach () > > endif () > > Use in WebInspectorUI/UserInterface/InspectorBackendCommands.js > in New Web Inspector. It was a wrong operation by mistake. :-( New Web Inspector uses WebInspectorUI/UserInterface/InspectorBackendCommands.js. So we dont need it. Hi, thanks for reviewing, all files from old inspector are no longer necessary and now i am handling only the necessary files of the new web inspector and all of them are being installed correctly. Do you guys think that we need some fix or is the patch good already? (In reply to comment #33) > Hi, thanks for reviewing, all files from old inspector are no longer necessary and now i am handling only the necessary files of the new web inspector and all of them are being installed correctly. > > Do you guys think that we need some fix or is the patch good already? My stance is that we don't need to keep unused files or code. (In reply to comment #34) > (In reply to comment #33) > > Hi, thanks for reviewing, all files from old inspector are no longer necessary and now i am handling only the necessary files of the new web inspector and all of them are being installed correctly. > > > > Do you guys think that we need some fix or is the patch good already? > > My stance is that we don't need to keep unused files or code. Yes, that's why i removed all references to old inspector. Now we are using only the code from new inspector. Sorry if i misunderstood, but can you please explain to me what is the problem now? :) Comment on attachment 210461 [details]
Fixed Changelog
Looks ok.
(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > Hi, thanks for reviewing, all files from old inspector are no longer necessary and now i am handling only the necessary files of the new web inspector and all of them are being installed correctly. > > > > > > Do you guys think that we need some fix or is the patch good already? > > > > My stance is that we don't need to keep unused files or code. > > Yes, that's why i removed all references to old inspector. Now we are using only the code from new inspector. > > Sorry if i misunderstood, but can you please explain to me what is the problem now? :) ok, looks nice. But, it would be good if seokju have a final review before landing. (In reply to comment #37) > (In reply to comment #35) > > (In reply to comment #34) > > > (In reply to comment #33) > > > > Hi, thanks for reviewing, all files from old inspector are no longer necessary and now i am handling only the necessary files of the new web inspector and all of them are being installed correctly. > > > > > > > > Do you guys think that we need some fix or is the patch good already? > > > > > > My stance is that we don't need to keep unused files or code. > > > > Yes, that's why i removed all references to old inspector. Now we are using only the code from new inspector. > > > > Sorry if i misunderstood, but can you please explain to me what is the problem now? :) > > ok, looks nice. But, it would be good if seokju have a final review before landing. Thank you Gyuyoung, if Seokju find some problem I'll be glad to fix it. I wonder that why you don't add 'inspectorPageIndex.html' for Remote web inspector. It seems that Remote web inspector works properly (without Bug 116587). And you could remove some tests related to inspector from TestExpectation. (In reply to comment #39) > I wonder that why you don't add 'inspectorPageIndex.html' for Remote web inspector. It seems that Remote web inspector works properly (without Bug 116587). > And you could remove some tests related to inspector from TestExpectation. The new inspector UI does not support the current inspector testing harness. So should we have all-inspector-tests skipped ? (In reply to comment #40) > (In reply to comment #39) > > I wonder that why you don't add 'inspectorPageIndex.html' for Remote web inspector. It seems that Remote web inspector works properly (without Bug 116587). > > And you could remove some tests related to inspector from TestExpectation. > > The new inspector UI does not support the current inspector testing harness. > So should we have all-inspector-tests skipped ? If this support for new inspector is not urgent, I prefer to support all inspector tests as well. Marcelo, did you check all inspector tests ? I clear r+ until verifying all inspector tests. (In reply to comment #41) > (In reply to comment #40) > > (In reply to comment #39) > > > I wonder that why you don't add 'inspectorPageIndex.html' for Remote web inspector. It seems that Remote web inspector works properly (without Bug 116587). > > > And you could remove some tests related to inspector from TestExpectation. > > > > The new inspector UI does not support the current inspector testing harness. > > So should we have all-inspector-tests skipped ? > > If this support for new inspector is not urgent, I prefer to support all inspector tests as well. Marcelo, did you check all inspector tests ? I clear r+ until verifying all inspector tests. Hi Seokju and Gyuyoung, yes I checked the tests and they are not supported for this new WebInspector. As Seokju already asked about it in the master bug (https://bugs.webkit.org/show_bug.cgi?id=118676) and as you can see in comment #7 the tests aren't skipped for now. The Windows port, already landed, didn't touched the tests (https://bugs.webkit.org/show_bug.cgi?id=120098), but GTK port, still under review, are skipping the tests (https://bugs.webkit.org/show_bug.cgi?id=120647). So I plan to touch the tests when a new test harness is available like all other platforms should do. What do you guys think? It's ok to do that or the correct is skip the tests expectations for now? See my comments about tests in Comment 9. (In reply to comment #43) > See my comments about tests in Comment 9. If we can't test existing inspector tests based on new inspector, I think we need to skip the tests for EFL port. (In reply to comment #44) > (In reply to comment #43) > > See my comments about tests in Comment 9. > > If we can't test existing inspector tests based on new inspector, I think we need to skip the tests for EFL port. Ok then, I'll skip the tests. But, due to local holidays I'll do it on monday. Created attachment 211105 [details]
patch
Skipping the tests for old inspector.
I also ran the tests for inspector-protocol and they have the same results for the old inspector and for the new inspector, some tests failed. I would like to create a specific bug for that and take a look at and try to fix the tests in a followup patch.
Comment on attachment 211105 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=211105&action=review > LayoutTests/platform/efl/TestExpectations:271 > +webkit.org/b/119559 inspector [ Skip ] Please file a new bug for solving this issue. Created attachment 211202 [details] patch Created a new bug for update TestExpectations when new tests are available for the new inspector (https://bugs.webkit.org/show_bug.cgi?id=121096) I also created another bug to check layout tests for inspector-protocol (https://bugs.webkit.org/show_bug.cgi?id=121094). I expect look at these bugs soon. Comment on attachment 211202 [details] patch Clearing flags on attachment: 211202 Committed r155475: <http://trac.webkit.org/changeset/155475> All reviewed patches have been landed. Closing bug. |