Thanks Jon, I uploaded a new versions here:
Spec: http://cr.openjdk.java.net/~hannesw/8220497/spec.02/ Webrev: http://cr.openjdk.java.net/~hannesw/8220497/webrev.02/ API docs: http://cr.openjdk.java.net/~hannesw/8220497/api.02/ As you found out, I already had uploaded .02 versions before with issues I found myself, but I wanted to wait for your review so I could upload these new versions which also address all the issues you found. I guess the simplest way to compare this webrev to the previous version is to look at the diff between the patches. Functional changes: - Whitespace-only search does not trigger query Test changes: - Fixed license header in javadoc-search.js to pure GPL2 - Added tests for whitespace only / no search - Fixed whitespace in javadoc-search.js Spec changes: - Use „Javadoc search“ capitalization consistently - Changed wording for parameter types in Core vs Peripheral Matches - Improve „Supported Browsers section Yes, the spec is only here for reference and will be a different changeset, there’s also a separate Jira issue for it. Hannes > Am 16.04.2019 um 22:10 schrieb Jonathan Gibbons <[email protected]>: > > In the Markdown spec, inconsistent capitalization of "javadoc search". > > We can't do much about it, but all those links to JLS contain a version > number that will have to be periodically updated. > > Core vs Peripheral Matches ... "Although a method's signature is not > considered a core component" ... earlier you defined a signature as including > the name, which is surely part of the signature. The rest of this sentence > "it is possible to search for method signatures by starting the search string > with (" suggests that you are trying to just refer to the method's > parameters. Also, in this sentence you focus on "methods" but elsewhere you > take care to differentiate "methods" and "constructors", suggesting that > constructors are not included here. > > White space: what is the rationale for the special case? I would have > expected the whitespace to be ignored by the "beginning or ending" clause in > the previous bullet, leaving an empty query string, which should be treated > as "no query". > > Supported Browsers ... it is "wishy-washy" to say that "it should work...". > That implies you don't actually know if it works in those browsers. I > suggest changing the sentence to "It is supported in the following browsers:" > The rows in the table seem to be randomly ordered. You give a version for > MSIE, but not for any other browsers. I'm surprised Chrome is limited to > Windows, as compared to "All OSes that support Chrome". > (I was reading spec.01, as given in the links below, but I see there are .02 > versions available. spec.02 does not contain the stylesheet. > > http://cr.openjdk.java.net/~hannesw/8220497/webrev.02/test/langtools/jdk/javadoc/doclet/testSearchScript/javadoc-search.js.html > Test has wrong license header ... tests should use plain GPL2, not GPL2+CE. > > New tests look good. If you fix the test license, the webrev is approved. I > presume the spec will be in a different changeset. > -- Jon > On 4/11/19 8:19 AM, Hannes Wallnöfer wrote: >> Thanks for the review, Jon. >> >> New spec/webrev/docs: >> >> Spec: >> http://cr.openjdk.java.net/~hannesw/8220497/spec.01/ >> >> Webrev: >> http://cr.openjdk.java.net/~hannesw/8220497/webrev.01/ >> >> API docs: >> http://cr.openjdk.java.net/~hannesw/8220497/api.01/ >> >> >> >> >>> Am 29.03.2019 um 21:34 schrieb Jonathan Gibbons >>> <[email protected]> >>> : >>> >>> >>> The spec: >>> The overview is a bit of a mix of spec and impl details. >>> >> I scrapped the overview and replaced it with a short rationale of why we >> need this spec, which is basically as an addendum to JEP 225 that defines >> the actual search behaviour. >> >> >> >>> Under Definitions >>> >>> I was surprised to see the new word "entity" until I realized it needs to >>> include both elements and custom searchable items. Maybe that should >>> be said somewhere. >>> >> Added. >> >> >>> "Upper case" and "lower case" are normally written as single words. >>> >>> >> Fixed. >> >> >>> Are we allowed non-letter characters in a camel-case search? i.e. should >>> the definition of camel-case include the non-alphabetic characters >>> >> I changed the definition of camel-case so it doesn’t sound like it lists all >> valid types of characters - that’ s what we refer to the JLS for. >> >> >>> Should "all-caps" (and "camel-case" include $ >>> >>> >> I removed all-caps since the underscore as word boundary now applies to all >> identifiers, see below. >> >> We already do support ‚$‘ in camel-case and partial matches. >> >> >> >>> Under Searchable Entities >>> >>> typo: entitis >>> >>> Under Types >>> >>> >>> Change "consist" to "consists" >>> >>> Under each of Packages, Types and Members >>> >>> >>> The current pattern is "The signature of <plural> consists of the ..." It >>> would be more correct to say "The signature of a <single-item> consists of >>> …“. >>> >>> >> All fixed. >> >> >> >>> Under Matching Rules/Left Boundaries: >>> >>> >>> 3rd bullet: why restrict letter following "_" to all-caps, and not after >>> "_" in all cases? >>> >> >> Great idea, I made that change and it works well. >> >> >> >>> Under Matching Rules/Camel case: >>> >>> >>> Wouldn't it be better to allow digits and other non-alpha characters, >>> treating them as the same as lower-case? >>> >>> >> Again, tweaked the text to make that clear. >> >> >> >>> Under Matching Rules/White space: >>> >>> >>> It seems a bug that typing excess whitespace should cause the match to >>> fail. You should ignore all whitespace everywhere in the search string. >>> >> I spent quite a bit of time on getting whitespace right. Ignoring all >> whitespace seems wrong as it would mean that „ob ject“ matches >> java.lang.Object. The solution I found is to make white space significant if >> it separates two words/identifiers, and make it optional if it occurs at the >> edge of the query string or before/after a separator character. >> >> This more or less replicates white space in Java syntax, and it happens to >> also work well for search tags. >> >> >> >>> Under Browser Requirements: >>> >>> >>> "Requirements" seems like the wrong word. How about "Supported Browsers", >>> with a note that it may work on other browsers as well. Also, you should >>> support Chrome on All OSs that support Chrome. >>> >>> >> Changed it to „supported browsers“. >> >> >>> Code >>> >>> I note the watermark is fixed in English. Is there any easy way to make it >>> localizable? >>> >>> >> That would have been part of my HTML5 placeholder attribute solution for >> JDK-8221366. It is still possible as things are, but a bit more cumbersome >> and I’d prefer to keep that separate from this issue. >> >> >>> General question, for later: should we publish/use a minified version of >>> search.js? >>> >>> >> I don’t think that’s necessary. The file is below 15 kb, so one of the >> smaller assets we load. >> >> Hannes >> >> >> >>> Other functionality questions are implicit in the comments on the spec. >>> >>> -- Jon >>>
