Closed Bug 770156 Opened 13 years ago Closed 10 years ago

GCLI needs a command to return the number of matches for a specified CSS selector

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Product:
DevTools Graveyard
Component:
Graphic Commandline and Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: miker, Assigned: pablo_lm1)

References

Details

(Whiteboard: [gclicommands])

Attachments

(1 file, 2 obsolete files)

qsa.patch
5.38 KB, patch
miker
: review+
Details | Diff | Splinter Review
New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
Attached patch Implemented the QSA command (obsolete) — — Splinter Review
Hey, I'm working on a fix, please **** it to me. I attached a possible fix.
Attachment #8682485 - Flags: feedback?(jwalker)
Comment on attachment 8682485 [details] [diff] [review] Implemented the QSA command Going to forward the f? to Mike again. Thanks Mike.
Attachment #8682485 - Flags: feedback?(jwalker) → feedback?(mratcliffe)
Assignee: nobody → pablo_lm1
Comment on attachment 8682485 [details] [diff] [review] Implemented the QSA command Review of attachment 8682485 [details] [diff] [review]: ----------------------------------------------------------------- We have a built-in type called 'nodelist' for use with commands that accept a querySelector. Changing to this means making a bunch of changes. When you finish updating the command can you ask me for review? ::: devtools/client/commandline/test/browser_cmd_qsa.js @@ +2,5 @@ > +* http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +// Tests that the qsa commands work as they should. > + > +const TEST_URI = "http://example.com/browser/devtools/client/commandline/"+ Please add "use strict"; above this line. Also, the file browser/devtools/client/commandline/test/browser_cmd_qsa.html is missing... you probably forgot to hg add it. No problem though, you can just use: const TEST_URI = "data:text/html;charset=utf-8,<body></body>"; @@ +12,5 @@ > + { > + setup: 'qsa', > + check: { > + input: 'qsa', > + hints: ' <query>', hints: ' [query]', @@ +13,5 @@ > + setup: 'qsa', > + check: { > + input: 'qsa', > + hints: ' <query>', > + markup: 'VVV' markup: 'VVV', status: 'VALID' @@ +17,5 @@ > + markup: 'VVV' > + } > + }, > + { > + setup: 'qsa a', There appears to be a bug when using the querySelector "a" in tests. For now we can use body instead: { setup: 'qsa body', check: { input: 'qsa body', hints: '', markup: 'VVVVVVVV', status: 'VALID' } } ::: devtools/shared/gcli/commands/qsa.js @@ +8,5 @@ > + > +exports.items = [ > + { > + item: "command", > + runAt: "client", runAt: "server", @@ +13,5 @@ > + name: "qsa", > + description: l10n.lookup("qsaDesc"), > + params: [{ > + name: "query", > + type: "string", type: nodelist @@ +16,5 @@ > + name: "query", > + type: "string", > + description: l10n.lookup("qsaQueryDesc") > + }], > + exec: function (args, context) { Please remove the space between function and ( @@ +17,5 @@ > + type: "string", > + description: l10n.lookup("qsaQueryDesc") > + }], > + exec: function (args, context) { > + let document = context.environment.document; All of this: let document = context.environment.document; let result = document.querySelectorAll(args.query); return result.length; Can be replaced with: return args.query.length; @@ +22,5 @@ > + let result = document.querySelectorAll(args.query); > + return result.length; > + } > + } > +] Missing semicolon
Attachment #8682485 - Flags: feedback?(mratcliffe)
Attached patch Updated command (obsolete) — — Splinter Review
Just applied the changes, thank you a lot for your feedback! There's something I don't understand though, regarding this: 'runAt: "server",'. If you don't mind explaining, why is the command run at server according to this? And what's the difference?
Attachment #8682485 - Attachment is obsolete: true
Attachment #8685686 - Flags: review?(mratcliffe)
Comment on attachment 8685686 [details] [diff] [review] Updated command Review of attachment 8685686 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Pablo Almenar from comment #6) > Created attachment 8685686 [details] [diff] [review] > Updated command > > Just applied the changes, thank you a lot for your feedback! > > There's something I don't understand though, regarding this: 'runAt: > "server",'. If you don't mind explaining, why is the command run at server > according to this? And what's the difference? Sure, these days Firefox is divided into two processes: 1. The process that controls the content of a web page such as DOM nodes. This is called the content or server process. 2. The process that controls the browser, so forward and back buttons, the developer toolbox etc. This is called the chrome or client process. These processes can only talk to one another in a limited way so the client process cannot directly access a DOM node from the server process. If you write a GCLI command and it doesn't need to touch content then you should use 'runAt: "client"' but if you need to access something in the content process you should use 'runAt: "server"'
Attachment #8685686 - Flags: review?(mratcliffe) → review+
backed this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5710245&repo=fx-team
Flags: needinfo?(pablo_lm1)
My fault... I had tested with all the changes I suggested but I meant to type this in my feedback: type: "nodelist", And not: type: nodelist, @Pablo: Can you fix this? It is in devtools/shared/gcli/commands/qsa.js I will send the patch to the try server to make sure it works on all platforms.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #11) > My fault... I had tested with all the changes I suggested but I meant to > type this in my feedback: > type: "nodelist", > > And not: > type: nodelist, > > @Pablo: Can you fix this? It is in devtools/shared/gcli/commands/qsa.js > > I will send the patch to the try server to make sure it works on all > platforms. Okay, I will get to fixing that typo now. I can send it to the try server myself as well, to see if it works before requesting review.
Flags: needinfo?(pablo_lm1)
(In reply to Pablo Almenar from comment #12) > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #11) > > My fault... I had tested with all the changes I suggested but I meant to > > type this in my feedback: > > type: "nodelist", > > > > And not: > > type: nodelist, > > > > @Pablo: Can you fix this? It is in devtools/shared/gcli/commands/qsa.js > > > > I will send the patch to the try server to make sure it works on all > > platforms. > > Okay, I will get to fixing that typo now. I can send it to the try server > myself as well, to see if it works before requesting review. Link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9dddd8d0fb6
Attached patch qsa.patch — — Splinter Review
I submitted the previous one with an old version of the repository. New link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42f14d8a36b6 It shows some failed/timed out tests but I think they are unrelated to my fix.
Attachment #8685686 - Attachment is obsolete: true
Attachment #8687594 - Flags: review?(mratcliffe)
That is a lot of failures and your patch seems fine. I have been looking into it and am not sure what is causing it... really, I don't have much time to look into the test issues but I have pushed to try once with and once without your patch: Without patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e160e94586c With patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5986a568f59 Let's see if it really is your patch that is triggering the failures.
fx-team was mostly orange for all devtools tests... until today. Let's push the patch again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e2482224a22
Attachment #8687594 - Flags: review?(mratcliffe) → review+
Try looks good, code looks good so more than happy to mark as checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b179a9db3f76
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: