RFR: 8220497: Improve Javadoc search feature and add test coverage

Wed Apr 24 20:31:22 UTC 2019

Looks good to me.

-- Jon

On 04/17/2019 08:48 AM, Hannes Wallnöfer wrote:
> 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 <jonathan.gibbons at oracle.com>:
>>
>> 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 <jonathan.gibbons at oracle.com>
>>>> :
>>>>
>>>>
>>>> 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
>>>>