Closed
Bug 1163961
Opened 10 years ago
Closed 9 years ago
Browser API: page search
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla41
| Tracking | Status | |
|---|---|---|
| firefox41 | --- | fixed |
People
(Reporter: paul, Assigned: paul)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 6 obsolete files)
|
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
We want to be able to search a string in the page.
|
Assignee | |
Updated•10 years ago
|
Blocks: browser-api
|
|
Assignee | |
Comment 1•10 years ago
|
||
Comment on attachment 8604591 [details] [diff] [review] wip Kanru, this is not finished, but can you take a quick look and tell me if I'm the right path (tested locally, it appears to work)?
Attachment #8604591 -
Flags: feedback?(kchen)
|
||
Comment 2•10 years ago
|
||
Comment on attachment 8604591 [details] [diff] [review] wip Review of attachment 8604591 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!
Attachment #8604591 -
Flags: feedback?(kchen) → feedback+
|
|
Assignee | |
Comment 3•9 years ago
|
||
|
|
Assignee | |
Comment 4•9 years ago
|
||
Attachment #8606974 -
Attachment is obsolete: true
Attachment #8607303 -
Flags: review?(kchen)
|
|
||
Comment 5•9 years ago
|
||
Comment on attachment 8607303 [details] [diff] [review] v1 Review of attachment 8607303 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: dom/browser-element/mochitest/browserElement_Find.js @@ +36,5 @@ > + ok(c(detail, { > + msg_name: "findchange", > + active: true, > + searchString: 'foo', > + searchLimit: 100, Read expected searchLimit from pref ::: dom/browser-element/mochitest/test_browserElement_oop_Find.html @@ +11,5 @@ > +</head> > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1163961">Mozilla Bug 1163961</a> > + > +<script type="application/javascript;version=1.7" src="browserElement_Find.js"> Should this be version=1.8? The _inproc_ test is version=1.8 ::: dom/html/nsBrowserElement.cpp @@ +384,5 @@ > + NS_ENSURE_TRUE_VOID(IsBrowserElementOrThrow(aRv)); > + NS_ENSURE_TRUE_VOID(IsNotWidgetOrThrow(aRv)); > + > + nsresult rv = mBrowserElementAPI->FindAll(aSearchString, aCaseSensitive); > + nit: trailing white spaces
Attachment #8607303 -
Flags: review?(kchen) → review+
|
|
||
Updated•9 years ago
|
Keywords: dev-doc-needed
|
|
Assignee | |
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0732e59f2d73
Attachment #8607303 -
Attachment is obsolete: true
Attachment #8608652 -
Flags: review+
|
|
Assignee | |
Comment 7•9 years ago
|
||
Note to self: - we need a DOM peer review - segfault: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0732e59f2d73
|
|
Assignee | |
Comment 8•9 years ago
|
||
Not segfault: INFO - JavaScript error: chrome://global/content/BrowserElementChildPreload.js, line 1214: TypeError: Finder is not a constructor
|
|
Assignee | |
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acbc5d0a6988
|
|
Assignee | |
Comment 10•9 years ago
|
||
I had to add `this.Finder = Finder` at the end of Finder.jsm because of how B2G load modules.
Attachment #8608652 -
Attachment is obsolete: true
Attachment #8616658 -
Flags: review+
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8616658 -
Flags: review?(ehsan)
|
||
Comment 12•9 years ago
|
||
Comment on attachment 8616658 [details] [diff] [review] v1 (r=kchen) Review of attachment 8616658 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/BrowserElement.webidl @@ +149,5 @@ > + > + [Throws, > + Pref="dom.mozBrowserFramesEnabled", > + CheckPermissions="browser"] > + void findAll(DOMString searchString, boolean caseSensitive); Please do not use boolean argument names, since seeing "true" or "false" at the call site doesn't help at all in showing what the argument does. Instead, use enums. For example: enum BrowserFindCaseSensitivity { "case-sensitive", "case-insensitive" }; [...] void findAll(DOMString searchString, BrowserFindCaseSensitivity caseSensitivity); @@ +154,5 @@ > + > + [Throws, > + Pref="dom.mozBrowserFramesEnabled", > + CheckPermissions="browser"] > + void findNext(boolean forward); Ditto.
Attachment #8616658 -
Flags: review?(ehsan) → review-
|
|
Assignee | |
Comment 13•9 years ago
|
||
I'm not sure about the way I'm juggling with IDL and webdidl data types. There might be a better way. Let me know.
Attachment #8616658 -
Attachment is obsolete: true
Attachment #8616864 -
Flags: review?(ehsan)
|
|
Assignee | |
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcd43c6474d3
|
|
||
Comment 15•9 years ago
|
||
Comment on attachment 8616864 [details] [diff] [review] v2 Review of attachment 8616864 [details] [diff] [review]: ----------------------------------------------------------------- r=me on BrowserElement.webidl. I also briefly looked at nsIBrowserElementAPI.idl, and using the numeric constants there is probably OK (but you could also use a string and pass in the WebIDL enum values there if you want to.) Let me know if you wanted a review of the other parts too.
Attachment #8616864 -
Flags: review?(ehsan) → review+
|
|
Assignee | |
Comment 16•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15) > Comment on attachment 8616864 [details] [diff] [review] > v2 > > Review of attachment 8616864 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me on BrowserElement.webidl. I also briefly looked at > nsIBrowserElementAPI.idl, and using the numeric constants there is probably > OK (but you could also use a string and pass in the WebIDL enum values there > if you want to.) Thanks you. > Let me know if you wanted a review of the other parts too. Kanru already reviewed the other parts, so I think we're good to go (once try server is fixed).
|
|
Assignee | |
Comment 17•9 years ago
|
||
With correct commit message.
Attachment #8616864 -
Attachment is obsolete: true
Attachment #8617719 -
Flags: review+
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 18•9 years ago
|
||
Comment on attachment 8617719 [details] [diff] [review] v2 (r=kchen,r=ehsan) Ehsan, I'm actually not sure you looked at the changes to the C++ code since I moved to a enum.
Attachment #8617719 -
Flags: review+ → review?(ehsan)
|
|
||
Comment 19•9 years ago
|
||
Comment on attachment 8617719 [details] [diff] [review] v2 (r=kchen,r=ehsan) Review of attachment 8617719 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsBrowserElement.h @@ +20,5 @@ > struct BrowserElementDownloadOptions; > class BrowserElementNextPaintEventCallback; > class DOMRequest; > +enum class BrowserFindCaseSensitivity: uint32_t; > +enum class BrowserFindDirection: uint32_t; These are fine!
Attachment #8617719 -
Flags: review?(ehsan) → review+
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8508ac3545
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb8508ac3545
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
|
||
Comment 22•9 years ago
|
||
Documented: https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/findAll https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/findNext https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/clearMatch
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•