Add basic introductory documentation for WebKit
Created attachment 409799 [details] Patch
Comment on attachment 409799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409799&action=review This is great!! Just a few minor comments so far: > Introduction.md:25 > +* **WebCore** - The largest component of WebKit, this layer which implements most of Web APIs and their behaviors. Nit - “this layer which implements” => “this layer implements” > Introduction.md:43 > + All WebContent processes in a single session (default vs. private browsing) shares a single networking session in the networking process. Nit - “shares” => “share” > Introduction.md:234 > +`Tools/Scripts/prepare-ChangeLog` script will create a stub entries for ChangeLog files based on code changes you made in your Git or Subversion checkouts. Nit - "will create stub entries" > Introduction.md:261 > +The âNo new tests. (OOPS!)â line appears if `prepare-ChangeLogs` did not detect the addition of new tests. Nit - `prepare-ChangeLogs` => `prepare-ChangeLog` > Introduction.md:296 > +Instead, add the same file in Sources.txt file that exist in each subdirectory of Source. Nit - "in the Sources.txt file that exists" > Introduction.md:304 > + Should we also mention the use of macros like WEBCORE_EXPORT to make private C++ methods and classes available across project boundaries? > Introduction.md:338 > +For code which should be enabled in iOS, watchOS, tvOS, and macOS Catalyst we use `PLATFORM(IOS_FAMILY)`. Nit - the official name is just “Mac Catalyst”. > Introduction.md:377 > +These results will like back to the test runs in buildbot which are associated with a specific failure. Nit - “like back” => “link back” > Introduction.md:486 > +Both `Parent` and `Child` are destructed when the last `Ref` or `RefPtr` to either object goes away. Nit - extra space before “to either object” > Introduction.md:676 > +Note that a single web [page](https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/Page.h) may consists of multiple documents Nit - “may consists of” => “may consist of”
Comment on attachment 409799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409799&action=review I reviewed part of this. > Introduction.md:6 App Store is two words. > Introduction.md:8 > +WebKit codebase is mostly written in C++ with bits of C and assembly primarily in JavaScriptCore, and some Objective-C to integrate with Cocoa platforms. "WebKit codebase" -> "The WebKit code base" or "WebKit code" "C and assembly primarily in JavaScriptCore," -> "C and assembly, primarily in JavaScriptCore," > Introduction.md:15 > + The rest of WebKit codebase is built using this template library instead of C++ STL. "STL" is an old fashioned name—should just call it the C++ standard library. I would not say we use WTF "instead of" the C++ standard library: we use a lot of the standard library. But I agree that in some cases we use the WTF classes instead of using certain things from the standard library. There must be a clearer way to say that. > Introduction.md:26 > + Itâs primarily concerned of three tree data structures: "concerned of" is not right here. You could say "concerned with", but even that is not quite it. > Introduction.md:29 > + * **CSS Object Model** - This is the result of parsing CSS. > + WebKit is also the only browser engine to implement [CSS JIT](https://webkit.org/blog/3271/webkit-css-selector-jit-compiler/). I am pretty sure that "CSS Object Model" is not the name of the internal parsed CSS. It’s the name of the web-exposed CSS data structures that we only create when needed. Also, the word "tree" isn’t mentioned here. > Introduction.md:30 > + * **Render Tree** - This tree represents the style of each element in DOM tree competed from CSS, and also stores the geometric layout information of each element. "competed" -> "computed" > Introduction.md:32 > + so that the rest of WebCore code can remain platform independent / agnostic across all the platforms WebKit runs: macOS, iOS, Windows, Linux, etc... "the platforms WebKit runs" -> "the platforms WebKit can run on" > Introduction.md:34 > + There is an ongoing multi-year project to slowly migrate code to PAL as we remove the reverse dependency to WebCore. "the reverse dependency" -> "any reverse dependencies" > Introduction.md:35 > +* **WebKitLegacy** (a.k.a. WebKit1) - This layer interfaces WebCore with the rest of operations systems in single process by implementing WebView on macOS and UIWebView on iOS. "operations systems" -> "operating systems"? "by implementing" -> "and implements" > Introduction.md:39 > + * **WebContent process** - This process loads & runs code loaded from websites. I think we should call it "web content process", not "WebContent process". > Introduction.md:48 > +[Bugzilla](https://bugs.webkit.org/) hosted on [webkit.org](http://webkit.org/) is the primary bug tracking tool we use. I prefer the name bugs.webkit.org — I know that Mozilla’s website and web app that we use on bugs.webkit.org is named Bugzilla, but I think ours is named bugs.webkit.org. > Introduction.md:51 > +### Code Reviews in Bugzilla bugs.webkit.org here and below > Introduction.md:67 > +Security bugs have their own components in Bugzilla. Weâre also working on a new policy to not land tests for security fixes. "to not land tests for security fixes" -> "to delay publishing tests for security fixes until after the fixes have been widely deployed" > Introduction.md:79 > +For connivence, you can add `Tools/Scripts/` to your path as follows in `~/.zshrc` like so: "connivence" -> "convenience" > Introduction.md:89 > +There is a script to update a WebKit: `Tools/Scripts/update-webkit`. This script will automatically merge change logs mentioned below. "a WebKit" -> "a WebKit checkout" > Introduction.md:98 > +macOS or Xcode, delete `WebKitBuild` directory. "`WebKitBuild` directory" -> "the `WebKitBuild` directory" > Introduction.md:99 > +`make clean` may not delete all the relevant files and may result in mysterious build or dyld errors. "may result" -> "building after doing that without deleting `WebKitBuild` may result"
(In reply to Wenson Hsieh from comment #2) > Comment on attachment 409799 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409799&action=review > > This is great!! Just a few minor comments so far: > > > Introduction.md:25 > > +* **WebCore** - The largest component of WebKit, this layer which implements most of Web APIs and their behaviors. > > Nit - “this layer which implements” => “this layer implements” Fixed. > > Introduction.md:43 > > + All WebContent processes in a single session (default vs. private browsing) shares a single networking session in the networking process. > > Nit - “shares” => “share” Fixed. > > Introduction.md:234 > > +`Tools/Scripts/prepare-ChangeLog` script will create a stub entries for ChangeLog files based on code changes you made in your Git or Subversion checkouts. > > Nit - "will create stub entries" Fixed. > > Introduction.md:261 > > +The âNo new tests. (OOPS!)â line appears if `prepare-ChangeLogs` did not detect the addition of new tests. > > Nit - `prepare-ChangeLogs` => `prepare-ChangeLog` Fixed. > > Introduction.md:296 > > +Instead, add the same file in Sources.txt file that exist in each subdirectory of Source. > > Nit - "in the Sources.txt file that exists" Fixed. > > Introduction.md:304 > > + > > Should we also mention the use of macros like WEBCORE_EXPORT to make private > C++ methods and classes available across project boundaries? Probably. Added a FIXME. > > Introduction.md:338 > > +For code which should be enabled in iOS, watchOS, tvOS, and macOS Catalyst we use `PLATFORM(IOS_FAMILY)`. > > Nit - the official name is just “Mac Catalyst”. Fixed. > > Introduction.md:377 > > +These results will like back to the test runs in buildbot which are associated with a specific failure. > > Nit - “like back” => “link back” Fixed. > > Introduction.md:486 > > +Both `Parent` and `Child` are destructed when the last `Ref` or `RefPtr` to either object goes away. > > Nit - extra space before “to either object” Fixed. > > Introduction.md:676 > > +Note that a single web [page](https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/Page.h) may consists of multiple documents > > Nit - “may consists of” => “may consist of” Fixed.
(In reply to Darin Adler from comment #3) > Comment on attachment 409799 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409799&action=review > > I reviewed part of this. > > > Introduction.md:6 > > > App Store is two words. Fixed. > > Introduction.md:8 > > +WebKit codebase is mostly written in C++ with bits of C and assembly primarily in JavaScriptCore, and some Objective-C to integrate with Cocoa platforms. > > "WebKit codebase" -> "The WebKit code base" or "WebKit code" Fixed to "the WebKit codebase". > "C and assembly primarily in JavaScriptCore," -> "C and assembly, primarily > in JavaScriptCore," Fixed. > > Introduction.md:15 > > + The rest of WebKit codebase is built using this template library instead of C++ STL. > > "STL" is an old fashioned name—should just call it the C++ standard library. Sure, fixed. > I would not say we use WTF "instead of" the C++ standard library: we use a > lot of the standard library. But I agree that in some cases we use the WTF > classes instead of using certain things from the standard library. There > must be a clearer way to say that. Okay, rephrased it as: "The rest of the WebKit codebase is built using this template library in addition to, and often in place of the equivalent template classes in, the C++ standard library." > > Introduction.md:26 > > + Itâs primarily concerned of three tree data structures: > > "concerned of" is not right here. You could say "concerned with", but even > that is not quite it. Okay, I've revised this whole section as follows: Most importantly, this components implement HTML, XML, and CSS parsers and implement HTML, SVG, and MathML elements as well as CSS. WebKit is also the only browser engine to implement [CSS JIT](https://webkit.org/blog/3271/webkit-css-selector-jit-compiler/). It works with a few tree data structures: * **Document Object Model** - This is the tree data structure we create from parsing HTML. * **Render Tree** - This tree represents the visual representation of each element in DOM tree computed from CSS and also stores the geometric layout information of each element. > > Introduction.md:32 > > + so that the rest of WebCore code can remain platform independent / agnostic across all the platforms WebKit runs: macOS, iOS, Windows, Linux, etc... > > "the platforms WebKit runs" -> "the platforms WebKit can run on" Fixed. > > Introduction.md:34 > > + There is an ongoing multi-year project to slowly migrate code to PAL as we remove the reverse dependency to WebCore. > > "the reverse dependency" -> "any reverse dependencies" Done. > > Introduction.md:35 > > +* **WebKitLegacy** (a.k.a. WebKit1) - This layer interfaces WebCore with the rest of operations systems in single process by implementing WebView on macOS and UIWebView on iOS. > > "operations systems" -> "operating systems"? Fixed. > "by implementing" -> "and implements" Fixed. > > Introduction.md:39 > > + * **WebContent process** - This process loads & runs code loaded from websites. > > I think we should call it "web content process", not "WebContent process". That would be inconsistent with the rest of the documentation so I'd keep this term for now. > > Introduction.md:48 > > +[Bugzilla](https://bugs.webkit.org/) hosted on [webkit.org](http://webkit.org/) is the primary bug tracking tool we use. > > I prefer the name bugs.webkit.org — I know that Mozilla’s website and web > app that we use on bugs.webkit.org is named Bugzilla, but I think ours is > named bugs.webkit.org. Okay. Replaced all instances of Bugzilla with bugs.webkit.org. > > Introduction.md:67 > > +Security bugs have their own components in Bugzilla. Weâre also working on a new policy to not land tests for security fixes. > > "to not land tests for security fixes" -> "to delay publishing tests for > security fixes until after the fixes have been widely deployed" Fixed. > > Introduction.md:79 > > +For connivence, you can add `Tools/Scripts/` to your path as follows in `~/.zshrc` like so: > > "connivence" -> "convenience" Fixed. > > Introduction.md:89 > > +There is a script to update a WebKit: `Tools/Scripts/update-webkit`. This script will automatically merge change logs mentioned below. > > "a WebKit" -> "a WebKit checkout" Oops, fixed. > > Introduction.md:98 > > +macOS or Xcode, delete `WebKitBuild` directory. > > "`WebKitBuild` directory" -> "the `WebKitBuild` directory" Fixed. > > Introduction.md:99 > > +`make clean` may not delete all the relevant files and may result in mysterious build or dyld errors. > > "may result" -> "building after doing that without deleting `WebKitBuild` > may result" Fixed.
Created attachment 409810 [details] Updated per review comments
Comment on attachment 409810 [details] Updated per review comments View in context: https://bugs.webkit.org/attachment.cgi?id=409810&action=review > Introduction.md:13 > + which segregates each type of object into its own page to prevent type confusion attacks upon UAF. I suggest writing "use after free" instead of "UAF". > Introduction.md:15 > + The rest of the WebKit codebase is built using this template library in addition to, and often in place of the equivalent template classes in, the C++ standard library. Misplaced commas here. Along with some wording changes, I would suggest: "in addition to, and often in place of, similar class templates in the C++ standard library" > Introduction.md:20 > + * **Interpreter** - This tier reads & executes instructions in byte code in C++. "&" -> "and" > Introduction.md:21 > + * **Baseline JIT** - The first JIT tier serves as the profiler as well as a significant speed up from interpreter. "from interpreter" -> "from the interpreter" > Introduction.md:22 > + * **DFG JIT** - Data Flow Graph Just in Time compiler uses the data flow analysis to generate the optimized machine code. "the optimized machine code" -> "optimized machine code" Also, I would capitalize "In". > Introduction.md:23 > + * **FTL JIT** - Faster than Light Just in Time compiler initially integrated with LLVM backend. I don’t know that "initially integrated with LLVM back end" is important to say. Ancient history. > Introduction.md:25 > +* **WebCore** - The largest component of WebKit, this layer implements most of Web APIs and their behaviors. "most web APIs" or "most of the web APIs" > Introduction.md:26 > + Most importantly, this components implement HTML, XML, and CSS parsers and implement HTML, SVG, and MathML elements as well as CSS. "this component implements" "and implements" > Introduction.md:80 > +For connivence, you can add `Tools/Scripts/` to your path as follows in `~/.zshrc` like so: "connivence" -> "convenience" > Introduction.md:86 > +This will allow you to run various tools you have by its name instead of typing the full path of the script. "various tools you have by its name" -> "various tools by name" > Introduction.md:114 > +You can also use Xcode to build & debug WebKit. Open `WebKit.xcworkspace` at the top level directory. "&" -> "and" > Introduction.md:147 > +* **Layout tests** - Resides in top-level [LayoutTests](https://trac.webkit.org/browser/webkit/trunk/LayoutTests) directory. I think it’s confusing to actually call these "layout tests" even though they are in a directory name "LayoutTests". As you say, this contains a variety of types of tests. I would call these the "WebKit tests" or the "Regression tests" or the "WebKit regression tests". The directory is named after the oldest type of regression test that we had when we first started creating this test suite. > Introduction.md:171 > +The WebKit project has no performance regression policy. -> The WebKit project has a "no performance regression" policy. The sentence above says that we have no policy! > Introduction.md:173 > +If your patch regresses one of these benchmarks even slightly (less than 1%), it would get reverted. "would get reverted" -> "will get reverted" > Introduction.md:175 > +* **JetStream2** - Measures JSCâs runtime performance. I would say "JavaScript performance" rather than "JSC's runtime performance". > Introduction.md:179 > +The following are benchmarks maintained by Apple's WebKit team but not shared due to licensing reasons. "not shared" -> "not available to other open source contributors" "due to licensing reasons" -> "because we don’t have the right to redistribute the content" (or something like that) Licensing reasons sounds like Apple is holding something back for its own reasons, but the reason is that we gathered content for these but we don’t have the right to redistribute it. Not sure how to best say it. But "due to licensing reasons" sounds wrong to me. > Introduction.md:189 > +WebKit has a rigorous code contribution process & policy in place to maintain the quality of code. "&" -> "and". > Introduction.md:194 > +You can run `Tools/Scripts/check-webkit-style` to check whether your code follows the coding guideline or not (this script isnât comprehensive). Not only is the script not comprehensive, but it’s not always correct. If it was a standard we would say "the script is non-normative". Not sure how to say that. > Introduction.md:196 > +it automatically runs the style checker against the code you changed so there is no need to run `check-webkit-style` manually. I would say "separately" rather than "manually". > Introduction.md:205 > +Use `--all-commands` to the list of all commands this utility tool supports. "utility tool" -> "tool". > Introduction.md:209 > +The majority of core WebKit uses LGPL. New code contributed to WebKit will use the [two clause BSD license](https://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE). Is it correct to say that the majority uses LGPL? If so I’d be surprised, but maybe. > Introduction.md:211 > +be sure to include the original copyright notice, which may be BSD or LGPL depending on a file. I don’t think this is worded correctly. You need to include the copyright notice for the moved code and also you should not change the license without the permission of the copyright holders. But the license is not the copyright notice. > Introduction.md:216 > +to make sure your code change doesnât break existing functionalities. "functionalities" is not right here; could say "functionality" or find a bette word. > Introduction.md:217 > +These days, uploading a patch on [bugs.webkit.org](https://bugs.webkit.org/) triggers Early Warning System (a.k.a. EWS) "Early Warning System" -> "the Early Warning System" > Introduction.md:221 > +then the rationale should be explained in the accompanying change log. I would be more specific "rationale" -> "reason a tests is impractical" > Introduction.md:271 > +Appleâs macOS, iOS, watchOS, and tvOS ports use Xcode and the rest uses [CMake](https://en.wikipedia.org/wiki/CMake). "rest uses" -> "rest use"
(In reply to Darin Adler from comment #7) > Comment on attachment 409810 [details] > Updated per review comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409810&action=review > > > Introduction.md:13 > > + which segregates each type of object into its own page to prevent type confusion attacks upon UAF. > > I suggest writing "use after free" instead of "UAF". Fixed. > > Introduction.md:15 > > + The rest of the WebKit codebase is built using this template library in addition to, and often in place of the equivalent template classes in, the C++ standard library. > > Misplaced commas here. Along with some wording changes, I would suggest: > > "in addition to, and often in place of, similar class templates in the C++ > standard library" Done. > > Introduction.md:20 > > + * **Interpreter** - This tier reads & executes instructions in byte code in C++. > > "&" -> "and" Replaced. > > Introduction.md:21 > > + * **Baseline JIT** - The first JIT tier serves as the profiler as well as a significant speed up from interpreter. > > "from interpreter" -> "from the interpreter" Fixed. > > Introduction.md:22 > > + * **DFG JIT** - Data Flow Graph Just in Time compiler uses the data flow analysis to generate the optimized machine code. > > "the optimized machine code" -> "optimized machine code" Fixed. > Also, I would capitalize "In". Sure, done. > > Introduction.md:23 > > + * **FTL JIT** - Faster than Light Just in Time compiler initially integrated with LLVM backend. > > I don’t know that "initially integrated with LLVM back end" is important to > say. Ancient history. Okay, revised it to: "Faster than Light Just In Time compiler which uses [B3 backend](https://webkit.org/blog/5852/introducing-the-b3-jit-compiler/). It’s the fastest tier of JSC." > > Introduction.md:25 > > +* **WebCore** - The largest component of WebKit, this layer implements most of Web APIs and their behaviors. > > "most web APIs" or "most of the web APIs" Fixed to say most of the Web APIs. > > Introduction.md:26 > > + Most importantly, this components implement HTML, XML, and CSS parsers and implement HTML, SVG, and MathML elements as well as CSS. > > "this component implements" > "and implements" Fixed. > > Introduction.md:80 > > +For connivence, you can add `Tools/Scripts/` to your path as follows in `~/.zshrc` like so: > > "connivence" -> "convenience" Fixed. > > Introduction.md:86 > > +This will allow you to run various tools you have by its name instead of typing the full path of the script. > > "various tools you have by its name" -> "various tools by name" Done. > > Introduction.md:114 > > +You can also use Xcode to build & debug WebKit. Open `WebKit.xcworkspace` at the top level directory. > > "&" -> "and" Fixed. > > Introduction.md:147 > > +* **Layout tests** - Resides in top-level [LayoutTests](https://trac.webkit.org/browser/webkit/trunk/LayoutTests) directory. > > I think it’s confusing to actually call these "layout tests" even though > they are in a directory name "LayoutTests". > > As you say, this contains a variety of types of tests. I would call these > the "WebKit tests" or the "Regression tests" or the "WebKit regression > tests". The directory is named after the oldest type of regression test that > we had when we first started creating this test suite. I agree with that sentiment but everyone else in the community calls them layout tests so I think we should stick with that terminology for now. Also, it now includes WPT tests as well so it's somewhat misleading to say WebKit regression tests at this point. I think Blink now calls it Web tests or so something like that. I think that's probably around the right name but we can revisit this sometime later. > > Introduction.md:171 > > +The WebKit project has no performance regression policy. > > -> The WebKit project has a "no performance regression" policy. > > The sentence above says that we have no policy! Oh oops, LOL. Fixed. That would be very misleading indeed. > > Introduction.md:173 > > +If your patch regresses one of these benchmarks even slightly (less than 1%), it would get reverted. > > "would get reverted" -> "will get reverted" Fixed. > > Introduction.md:175 > > +* **JetStream2** - Measures JSCâs runtime performance. > > I would say "JavaScript performance" rather than "JSC's runtime performance". Oh, right. Fixed. I guess JavaScript and WASM performance. > > Introduction.md:179 > > +The following are benchmarks maintained by Apple's WebKit team but not shared due to licensing reasons. > > "not shared" -> "not available to other open source contributors" > > "due to licensing reasons" -> "because we don’t have the right to > redistribute the content" (or something like that) > > Licensing reasons sounds like Apple is holding something back for its own > reasons, but the reason is that we gathered content for these but we don’t > have the right to redistribute it. Not sure how to best say it. But "due to > licensing reasons" sounds wrong to me. Oh I see, that's a good point. Revised it to say: "The following are benchmarks maintained by Apple's WebKit team but not available to other open source contributors since Apple doesn't have the right to redistribute the content." > > Introduction.md:189 > > +WebKit has a rigorous code contribution process & policy in place to maintain the quality of code. > > "&" -> "and". Fixed. > > Introduction.md:194 > > +You can run `Tools/Scripts/check-webkit-style` to check whether your code follows the coding guideline or not (this script isnât comprehensive). > > Not only is the script not comprehensive, but it’s not always correct. If it > was a standard we would say "the script is non-normative". Not sure how to > say that. Hm... okay. Revised it to say "You can run `Tools/Scripts/check-webkit-style` to check whether your code follows the coding guideline or not (it can report false positives or false negatives)." > > Introduction.md:196 > > +it automatically runs the style checker against the code you changed so there is no need to run `check-webkit-style` manually. > > I would say "separately" rather than "manually". Sure, done. > > Introduction.md:205 > > +Use `--all-commands` to the list of all commands this utility tool supports. > > "utility tool" -> "tool". Done. > > Introduction.md:209 > > +The majority of core WebKit uses LGPL. New code contributed to WebKit will use the [two clause BSD license](https://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE). > > Is it correct to say that the majority uses LGPL? If so I’d be surprised, > but maybe. Oh, I meant to say WebCore, not WebKit. Revised this to say: Much of the code we inherited from [KHTML](https://en.wikipedia.org/wiki/KHTML) is licendered under [LGPL](https://en.wikipedia.org/wiki/GNU_Lesser_General_Public_License). New code contributed to WebKit will use the [two clause BSD license](https://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE). > > Introduction.md:211 > > +be sure to include the original copyright notice, which may be BSD or LGPL depending on a file. > > I don’t think this is worded correctly. You need to include the copyright > notice for the moved code and also you should not change the license without > the permission of the copyright holders. But the license is not the > copyright notice. Good point. Revised to say: "When moving the existing code, you need to include the original copyright notice for the moved code and you should also not change the license, which may be BSD or LGPL depending on a file, without the permission of the copyright holders." > > Introduction.md:216 > > +to make sure your code change doesnât break existing functionalities. > > "functionalities" is not right here; could say "functionality" or find a > bette word. Changed to functionality. > > Introduction.md:217 > > +These days, uploading a patch on [bugs.webkit.org](https://bugs.webkit.org/) triggers Early Warning System (a.k.a. EWS) > > "Early Warning System" -> "the Early Warning System" Fixed. > > Introduction.md:221 > > +then the rationale should be explained in the accompanying change log. > > I would be more specific > > "rationale" -> "reason a tests is impractical" Revised to say: "then the reason writing a tests is impractical should be explained in the accompanying change log." > > Introduction.md:271 > > +Appleâs macOS, iOS, watchOS, and tvOS ports use Xcode and the rest uses [CMake](https://en.wikipedia.org/wiki/CMake). > > "rest uses" -> "rest use" Fixed.
Created attachment 409815 [details] Updated 2
Comment on attachment 409815 [details] Updated 2 View in context: https://bugs.webkit.org/attachment.cgi?id=409815&action=review I gave up reading part-way through. This is really long; maybe it should be broken into several files? > Introduction.md:19 > + JSC parses JavaScript and generates byte code, which is then executed by one of the following four tiers. > + Many tiers are needed to balance between compilation time and execution time. It also exposes JavaScriptCore API for macOS and iOS applications. Need re-ordering; two sentences about tiers, then the unrelated "It also exposes". > Introduction.md:25 > +* **WebCore** - The largest component of WebKit, this layer implements most of. the Web APIs and their behaviors. of. > Introduction.md:26 > + Most importantly, this component implements HTML, XML, and CSS parsers and implements HTML, SVG, and MathML elements as well as CSS. Is XML parsing here or via libxml? > Introduction.md:27 > + WebKit is also the only browser engine to implement [CSS JIT](https://webkit.org/blog/3271/webkit-css-selector-jit-compiler/). Seems a little out of place. > Introduction.md:68 > +Weâre also working on a new policy to delay publishing tests for security fixes until after the fixes have been widely deployed. Is this appropriate here? > Introduction.md:83 > +export PATH=$PATH:/Volumes/Data/webkit/Tools/Scripts/:$PATH You should probably say "replacing `Volumes/Data/webkit` with a path to your checkout". > Introduction.md:96 > +### Removing WebKitBuild directory This should be "how to resolve build failures' (describe the problem not the solution). > Introduction.md:103 > +### Building ASan This section should be "how to investigate memory corruption bugs" or something. > Introduction.md:274 > +While there used to be 11 distinct build systems used in WebKit, weâve reduced that down to just two: The historical comment doesn't seem relevant here.
(In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 409815 [details] > Updated 2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409815&action=review > > I gave up reading part-way through. This is really long; maybe it should be > broken into several files? This document is meant to include everything you need to know in order to start writing a WebKit patch. The fact it's so long is an evidence that our codebase and processes are way too complicated; i.e. it's a symptom and not the cause. We should really have some project to streamline our codebase so that new comers don't need to learn so many random stuff. If we split it into multiple documents, then whoever reading this document would have to find & connect all those different bits and pieces. Frankly, this is why our wiki failed. There are so many documents about random bits and pieces about our codebase and process and there is no coherency whatsoever. > > Introduction.md:19 > > + JSC parses JavaScript and generates byte code, which is then executed by one of the following four tiers. > > + Many tiers are needed to balance between compilation time and execution time. It also exposes JavaScriptCore API for macOS and iOS applications. > > Need re-ordering; two sentences about tiers, then the unrelated "It also > exposes". Sure, rephrased it as "JavaScriptCode also implements JavaScriptCore API for macOS and iOS applications." and moved it after the bullet lists. > > Introduction.md:25 > > +* **WebCore** - The largest component of WebKit, this layer implements most of. the Web APIs and their behaviors. > > of. Oops, removed "." > > Introduction.md:26 > > + Most importantly, this component implements HTML, XML, and CSS parsers and implements HTML, SVG, and MathML elements as well as CSS. > > Is XML parsing here or via libxml? Yes. libxml parses XML text but we build DOM out of it. Also, the fact we use a third party library as a part of our parser is more or less implementation detail. We used to use bison to generate CSS parser but we would still say we have a CSS parser. > > Introduction.md:27 > > + WebKit is also the only browser engine to implement [CSS JIT](https://webkit.org/blog/3271/webkit-css-selector-jit-compiler/). > > Seems a little out of place. Since this documentation is meant for new comers, I want them to get excited about WebKit as well. I've rephrased it as "It also implements [CSS JIT](https://webkit.org/blog/3271/webkit-css-selector-jit-compiler/), the only Just In Time compiler for CSS in existence." > > Introduction.md:68 > > +Weâre also working on a new policy to delay publishing tests for security fixes until after the fixes have been widely deployed. > > Is this appropriate here? I think so. We don't want people to be landing security bug fixes with a test. > > Introduction.md:83 > > +export PATH=$PATH:/Volumes/Data/webkit/Tools/Scripts/:$PATH > > You should probably say "replacing `Volumes/Data/webkit` with a path to your > checkout". Sure, added "where `/Volumes/Data/webkit` is the path to a WebKit checkout." below this. > > Introduction.md:96 > > +### Removing WebKitBuild directory > > This should be "how to resolve build failures' (describe the problem not the > solution). Good point. Renamed it to "Fixing mysterious build or runtime errors after Xcode upgrades" > > Introduction.md:103 > > +### Building ASan > > This section should be "how to investigate memory corruption bugs" or > something. Good suggestion. Renamed it to "Building with Address Sanitizer to investigate memory corruption bugs" > > Introduction.md:274 > > +While there used to be 11 distinct build systems used in WebKit, weâve reduced that down to just two: > > The historical comment doesn't seem relevant here. Yeah, this whole paragraph is very wordy. Revised it to: Apple’s macOS, iOS, watchOS, and tvOS ports use Xcode and the rest use [CMake](https://en.wikipedia.org/wiki/CMake) to build WebKit. There is an ongoing effort to make Apple's ports also use CMake.
Created attachment 409848 [details] Updated 3
Comment on attachment 409848 [details] Updated 3 View in context: https://bugs.webkit.org/attachment.cgi?id=409848&action=review > Introduction.md:12 > +* **bmalloc** - WebKitâs malloc implementation as a bump pointer allocator. It provides an important security feature, called IsoHeap, What is "a bump pointer allocator"? Can you put a hyperlink? > Introduction.md:25 > + JavaScriptCode also implements JavaScriptCore API for macOS and iOS applications. JavaScriptCode? JavaScriptCore or JavaScript code? > Introduction.md:148 > +* **JavaScript tests** - Resides in top-level JSTests directory. Only this one isn't hyperlinked. [JSTests](https://trac.webkit.org/browser/webkit/trunk/JSTests) > Introduction.md:214 > +Much of the code we inherited from [KHTML](https://en.wikipedia.org/wiki/KHTML) is licendered under [LGPL](https://en.wikipedia.org/wiki/GNU_Lesser_General_Public_License). licendered → licensed > Introduction.md:245 > +You should edit these stubs to describe your change, including the full url to the bug (example entry, note that you can use `--bug` flag). url → URL > Introduction.md:281 > +We call this mechanism unified *Unified Sources* or *Unified Builds*. "unified Unified Sources" → "Unified Sources" ? > Introduction.md:313 > +FIXME: Mention WPT_EXPORT and WEBCORE_EXPORT. WPT_EXPORT → WTF_EXPORT_PRIVATE ? > Introduction.md:400 > +To be written in the future (11/11/2019) Add "FIXME:" to align with other sections. > Introduction.md:435 > +There is a convenience super template class, convenience → convenient ? > Introduction.md:715 > +As a result, a single C++ object may have multiple JS wrappers in distant `DOMWrapperWorld`s. distant → distinct ? > Introduction.md:744 > + the [marking pharse](https://en.wikipedia.org/wiki/Tracing_garbage_collection#Basic_algorithm), pharse → phase > Introduction.md:905 > +> If Safari fails to launch, specify `--no-show-results` and open results.html file manually. Do you need ">" here? Is this a quote? > Introduction.md:951 > +These tests also have accompanying `-expected.png` files but `run-webkit-tests` doesn't check the PNG output againt the expected result by default. againt → against > Introduction.md:958 > +### dumpAsText test Do we still need dumpAsText test? Can js-test.js replace old dumpAsText tests? > Introduction.md:1016 > +<script src="../../../resources/js-test-pre.js"></script> Why do you choose an old js-test-pre.js test as a example of a new document? > Introduction.md:1068 > +* `finishJSTest()` - When js-test.js style test needs to do some async work, define the global variable named jsTestIsAsync and set it to true. When the test is done, call this function to notify the test runner (donât call `testRunner.notifyDone` mentioned later directly). See [an example](https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/dom/iframe-innerWidth.html). Why do you choose an old js-test-pre.js test as a example of a new document? > Introduction.md:1077 > + `Tools/Scripts/make-new-script-test` fast/dom/new-test.html Remove the "`". > Introduction.md:1159 > +> As mentioned above, do not modify tests in [LayoutTests/imported/w3c/web-platform-tests](https://trac.webkit.org/browser/webkit/trunk/LayoutTests/imported/w3c/web-platform-tests) Do you need ">" here? Is this a quote? > Introduction.md:1172 > +* The file with the same name as the test name except it ends with `-expected-match.*` or `-ref.*` is a matching expected result for the test. -expected-match.* → -expected.* > Introduction.md:1173 > +* The file with the same name as the test name except it ends with `-expected-mismatch.*` or `-ref.*` is a matching expected result for the test. "-ref.* is a matching" → "-notref.* is a mismatching" ? > Introduction.md:1271 > +[https://webkit-test-results.webkit.org/](https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html) dead link. "flakiness dashboard" was replaced by https://results.webkit.org/ > Introduction.md:1283 > + Sceenshot → Screenshot > Introduction.md:1286 > + Sceenshot → Screenshot
Comment on attachment 409848 [details] Updated 3 View in context: https://bugs.webkit.org/attachment.cgi?id=409848&action=review > Introduction.md:852 > +FIXME: Explain how test expectations work. Link to this? https://trac.webkit.org/wiki/TestExpectations
(In reply to Fujii Hironori from comment #13) > Comment on attachment 409848 [details] > Updated 3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409848&action=review > > > Introduction.md:12 > > +* **bmalloc** - WebKitâs malloc implementation as a bump pointer allocator. It provides an important security feature, called IsoHeap, > > What is "a bump pointer allocator"? Can you put a hyperlink? I couldn't find a good resource for this. You can read about it in https://hacks.mozilla.org/2019/03/fast-bump-allocated-virtual-doms-with-rust-and-wasm/ and https://github.com/fitzgen/bumpalo but I'm not gonna add a hyperlink here because these aren't really descriptions of what a bump allocation is. > > Introduction.md:25 > > + JavaScriptCode also implements JavaScriptCore API for macOS and iOS applications. > > JavaScriptCode? JavaScriptCore or JavaScript code? JavaScriptCore implements JavaScriptCore API. > > Introduction.md:148 > > +* **JavaScript tests** - Resides in top-level JSTests directory. > > Only this one isn't hyperlinked. > [JSTests](https://trac.webkit.org/browser/webkit/trunk/JSTests) Fixed. > > Introduction.md:214 > > +Much of the code we inherited from [KHTML](https://en.wikipedia.org/wiki/KHTML) is licendered under [LGPL](https://en.wikipedia.org/wiki/GNU_Lesser_General_Public_License). > > licendered → licensed Fixed. > > Introduction.md:245 > > +You should edit these stubs to describe your change, including the full url to the bug (example entry, note that you can use `--bug` flag). > > url → URL Fixed although both forms are used these days. > > Introduction.md:281 > > +We call this mechanism unified *Unified Sources* or *Unified Builds*. > > "unified Unified Sources" → "Unified Sources" ? Oops, fixed. > > Introduction.md:313 > > +FIXME: Mention WPT_EXPORT and WEBCORE_EXPORT. > > WPT_EXPORT → WTF_EXPORT_PRIVATE ? Fixed. > > Introduction.md:400 > > +To be written in the future (11/11/2019) > > Add "FIXME:" to align with other sections. Done. > > Introduction.md:435 > > +There is a convenience super template class, > > convenience → convenient ? No, convenience is adjective here. > > Introduction.md:715 > > +As a result, a single C++ object may have multiple JS wrappers in distant `DOMWrapperWorld`s. > > distant → distinct ? Fixed. > > Introduction.md:744 > > + the [marking pharse](https://en.wikipedia.org/wiki/Tracing_garbage_collection#Basic_algorithm), > > pharse → phase Fixed. > > Introduction.md:905 > > +> If Safari fails to launch, specify `--no-show-results` and open results.html file manually. > > Do you need ">" here? Is this a quote? It's really meant as a note. > > Introduction.md:951 > > +These tests also have accompanying `-expected.png` files but `run-webkit-tests` doesn't check the PNG output againt the expected result by default. > > againt → against Fixed. > > Introduction.md:958 > > +### dumpAsText test > > Do we still need dumpAsText test? Can js-test.js replace old dumpAsText > tests? No. dumpAsText is used for more than just js-test.js tests. > > Introduction.md:1016 > > +<script src="../../../resources/js-test-pre.js"></script> > > Why do you choose an old js-test-pre.js test as a example of a new document? Because we have thousands of them and people need to be able to read & modify existing tests. > > Introduction.md:1068 > > +* `finishJSTest()` - When js-test.js style test needs to do some async work, define the global variable named jsTestIsAsync and set it to true. When the test is done, call this function to notify the test runner (donât call `testRunner.notifyDone` mentioned later directly). See [an example](https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/dom/iframe-innerWidth.html). > > Why do you choose an old js-test-pre.js test as a example of a new document? Again, people need to be able to read existing tests. > > Introduction.md:1077 > > + `Tools/Scripts/make-new-script-test` fast/dom/new-test.html > > Remove the "`". Oops, another quip-ism. Fixed. > > Introduction.md:1159 > > +> As mentioned above, do not modify tests in [LayoutTests/imported/w3c/web-platform-tests](https://trac.webkit.org/browser/webkit/trunk/LayoutTests/imported/w3c/web-platform-tests) > > Do you need ">" here? Is this a quote? Yes, it's really a note. > > Introduction.md:1172 > > +* The file with the same name as the test name except it ends with `-expected-match.*` or `-ref.*` is a matching expected result for the test. > > -expected-match.* → -expected.* Oops, fixed. > > Introduction.md:1173 > > +* The file with the same name as the test name except it ends with `-expected-mismatch.*` or `-ref.*` is a matching expected result for the test. > > "-ref.* is a matching" → "-notref.* is a mismatching" ? Oops, fixed. > > Introduction.md:1271 > > +[https://webkit-test-results.webkit.org/](https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html) > > dead link. "flakiness dashboard" was replaced by https://results.webkit.org/ Updated. > > Introduction.md:1283 > > + > > Sceenshot → Screenshot Fixed. > > Introduction.md:1286 > > + > > Sceenshot → Screenshot Ditto.
Created attachment 409869 [details] Updated 4
(In reply to Fujii Hironori from comment #14) > Comment on attachment 409848 [details] > Updated 3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409848&action=review > > > Introduction.md:852 > > +FIXME: Explain how test expectations work. > > Link to this? https://trac.webkit.org/wiki/TestExpectations That page is quite a bit outdated. I'd rather not point people to outdated documents.
Comment on attachment 409869 [details] Updated 4 View in context: https://bugs.webkit.org/attachment.cgi?id=409869&action=review Amazing and hugely helpful! Thanks for putting this together! > Introduction.md:37 > +* **WebKit** (a.k.a. WebKit2) - This layer implements multi-process architecture of WebKit, and implements WKWebView on macOS and iOS. Nit: s/multi-process/the multi-process > Introduction.md:60 > +This allow contributors to find build or test failures before committing code changes to the WebKitâs primary Subversion repository. s/allow/allows > Introduction.md:65 > + Does the WebKit community have a preferred commit message style? The Mozilla community, for instance, has a strict about commit messages/format (e.g., "Bug XXX - commit message"). > Introduction.md:127 > + It maybe worth mentioning something about all the l10n warnings in Xcode? Also, Xcode seems to add random files into the tree... might be worth mentioning how to deal with those safely? > Introduction.md:198 > +You can run `Tools/Scripts/check-webkit-style` to check whether your code follows the coding guideline or not s/guideline/guidelines > Introduction.md:211 > + Can anyone use this or does this require any special credentials? > Introduction.md:320 > +This happens because if one `UnifiedSource1.cpp` contained `a.cpp`, `b.cpp`, `c.cpp`, then `#include` in `a.cpp` could have pulled in some header files `c.cpp` needed. s/header files/header files that/ > Introduction.md:390 > + Is there some guidelines on who can push to CI? Can one push to CI? Can CI be configured to build and run specific tests? > Introduction.md:469 > +use R value reference with `WTFMove` (equivalent to `std::move` with some safety checks), s/R value/rvalue > Introduction.md:651 > +[Document Object Model](https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model) Maybe pointing to https://dom.spec.whatwg.org might better here? Expectation being that a contributor should be looking at the specs, not web developer docs. > Introduction.md:653 > +It consists of one or more instances of subclasses of [Node](https://developer.mozilla.org/en-US/docs/Web/API/Node) As above - https://dom.spec.whatwg.org/#nodes > Introduction.md:673 > +per the way [HTML5 parser](https://html.spec.whatwg.org/multipage/parsing.html#parsing) is specified. s/HTML5/HTML > Introduction.md:732 > + It would be great to have a section on WebIDL bindings.... like, where does one put WebIDL in the tree? Where does the generated C++ end up? > Introduction.md:775 > +In order to safeguard the rest of the system and keep the application remain responsive s/and keep the application remain responsive/and allow the application to remain responsive
Comment on attachment 409869 [details] Updated 4 View in context: https://bugs.webkit.org/attachment.cgi?id=409869&action=review Thank you for your this huge effort. This is pretty helpful. > Introduction.md:19 > + Many tiers are needed to balance between compilation time and execution time. This might be more effective to have the link to https://webkit.org/blog/10308/speculation-in-javascriptcore/ > Introduction.md:52 > +### Code Reviews in bugs.webkit.org I seem it would be nice to have the link to https://webkit.org/commit-and-review-policy/ > Introduction.md:695 > +## JavaScript Wrappers I'd like to read a description about `ActiveDOMObject`. Or add the link to https://trac.webkit.org/wiki/DOMBindingsEventLoopNotes ?
(In reply to Marcos Caceres from comment #18) > Comment on attachment 409869 [details] > Updated 4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409869&action=review > > Amazing and hugely helpful! Thanks for putting this together! > > > Introduction.md:37 > > +* **WebKit** (a.k.a. WebKit2) - This layer implements multi-process architecture of WebKit, and implements WKWebView on macOS and iOS. > > Nit: s/multi-process/the multi-process Fixed. > > Introduction.md:60 > > +This allow contributors to find build or test failures before committing code changes to the WebKitâs primary Subversion repository. > > s/allow/allows Fixed. > > Introduction.md:65 > > + > Does the WebKit community have > Does the WebKit community have a preferred commit message style? The Mozilla > community, for instance, has a strict about commit messages/format (e.g., > "Bug XXX - commit message"). The commit message should be made by commit-log-editor based on change logs. If you ran setup-git-clone, it would automatically do it for you. But this is somewhat irrelevant until you get a Subversion account setup. The commit queue will automatically generate one from the change logs. I've added the following: The Subvesion commit message should be created by `Tools/Scripts/commit-log-editor` based on the change log entries. `Tools/Scripts/webkit-patch land` does this automatically. > > Introduction.md:127 > > + > > It maybe worth mentioning something Does the WebKit community havementioning how to deal with those safely? Huh, never seen those. Let's add that later though. This is taking a long time to get through the review process already. > > Introduction.md:198 > > +You can run `Tools/Scripts/check-webkit-style` to check whether your code follows the coding guideline or not > > s/guideline/guidelines Fixed. > > Introduction.md:211 > > + > > Can anyone use this or does this require any special credentials? Anyone can use them. Things like `land` obviously require committer privileges but that's not because the script refuses to run. It's more that the commit queue will reject a request from a non-committer. > > Introduction.md:320 > > +This happens because if one `UnifiedSource1.cpp` contained `a.cpp`, `b.cpp`, `c.cpp`, then `#include` in `a.cpp` could have pulled in some header files `c.cpp` needed. > > s/header files/header files that/ Fixed. > > Introduction.md:390 > > + > > Is there some guidelines on who can push to CI? Can one push to CI? Can CI > be configured to build and run specific tests? Patches get pushed to build.webkit.org once it's committed to the WebKit repository's trunk branch. > > Introduction.md:469 > > +use R value reference with `WTFMove` (equivalent to `std::move` with some safety checks), > > s/R value/rvalue Fixed. > > Introduction.md:651 > > +[Document Object Model](https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model) > > Maybe pointing to https://dom.spec.whatwg.org might better here? Expectation > being that a contributor should be looking at the specs, not web developer > docs. Well, I find MDN to be a better resource when someone isn't even familiar with DOM. If someone is already familiar with DOM and such, there is a link to the specification in MDN anyway. > > Introduction.md:653 > > +It consists of one or more instances of subclasses of [Node](https://developer.mozilla.org/en-US/docs/Web/API/Node) > > As above - https://dom.spec.whatwg.org/#nodes Ditto. > > Introduction.md:673 > > +per the way [HTML5 parser](https://html.spec.whatwg.org/multipage/parsing.html#parsing) is specified. > > s/HTML5/HTML Sure, changed. > > Introduction.md:732 > > + > > It would be great to have a section on WebIDL bindings.... like, where does > one put WebIDL in the tree? Where does the generated C++ end up? Sure. Added a FIXME. > > Introduction.md:775 > > +In order to safeguard the rest of the system and keep the application remain responsive > > s/and keep the application remain responsive/and allow the application to > remain responsive Fixed.
(In reply to Tetsuharu Ohzeki from comment #19) > Comment on attachment 409869 [details] > Updated 4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409869&action=review > > Thank you for your this huge effort. This is pretty helpful. > > > Introduction.md:19 > > + Many tiers are needed to balance between compilation time and execution time. > > This might be more effective to have the link to > https://webkit.org/blog/10308/speculation-in-javascriptcore/ Good idea. Added. > > Introduction.md:52 > > +### Code Reviews in bugs.webkit.org > > I seem it would be nice to have the link to > https://webkit.org/commit-and-review-policy/ I'm trying to avoid replying on those external documents for now to streamline the reading material. What exactly did you find it useful in that document? > > Introduction.md:695 > > +## JavaScript Wrappers > > I'd like to read a description about `ActiveDOMObject`. > Or add the link to https://trac.webkit.org/wiki/DOMBindingsEventLoopNotes ? Ditto. My plan is to explain all those things in this document. As is, that meeting note doesn't do the justice so I'd leave it out for now.
Created attachment 409938 [details] Updated 5
(In reply to Ryosuke Niwa from comment #21) > > > Introduction.md:52 > > > +### Code Reviews in bugs.webkit.org > > > > I seem it would be nice to have the link to > > https://webkit.org/commit-and-review-policy/ > > I'm trying to avoid replying on those external documents for now to > streamline the reading material. What exactly did you find it useful in that > document? I thought this document lacked to describe about some grants & roles (bugzilla's edit, comitter, reviewer) in spite of this document tell them. They are also a scope of this document, aren't them?
(In reply to Tetsuharu Ohzeki from comment #23) > (In reply to Ryosuke Niwa from comment #21) > > > > Introduction.md:52 > > > > +### Code Reviews in bugs.webkit.org > > > > > > I seem it would be nice to have the link to > > > https://webkit.org/commit-and-review-policy/ > > > > I'm trying to avoid replying on those external documents for now to > > streamline the reading material. What exactly did you find it useful in that > > document? > > I thought this document lacked to describe about some grants & roles > (bugzilla's edit, comitter, reviewer) in spite of this document tell them. > > They are also a scope of this document, aren't them? Alright, I'm gonna add "Contributing to WebKit" and "Filing a bug and editing bugs" to cover those topics.
Created attachment 409959 [details] Updated 6
(In reply to Ryosuke Niwa from comment #11) > This document is meant to include everything you need to know in order to > start writing a WebKit patch. The fact it's so long is an evidence that our > codebase and processes are way too complicated; i.e. it's a symptom and not > the cause. We should really have some project to streamline our codebase so > that new comers don't need to learn so many random stuff. If we split it > into multiple documents, then whoever reading this document would have to > find & connect all those different bits and pieces. Frankly, this is why our > wiki failed. There are so many documents about random bits and pieces about > our codebase and process and there is no coherency whatsoever. I'm slightly skeptical about adding a new introduction.md without formally getting rid of the wiki (and, as you say, the wiki is a mess). We already have documentation split across at least three places (Readme.md, trac.webkit.org/wiki, and webkit.org) and it's not really clear how to get the necessary permissions to edit each of these if the documentation is a mess. We need to have some real discussion about how to document WebKit going forward, which I've spoken a bit with a few people on about (mostly recently Brady and Maciej), and I'm unconvinced that adding a single, huge page to the root of the repo is a good approach. The biggest problem with the wiki is lack of coherency and lack of any real cross-references. A single page doesn't solve the former (especially as it gets longer), and the lack of cross-references is only mitigated by Cmd/Ctrl+F. That said, I think this is too long; I don't think you need everything in here to write a WebKit patch. An introduction to every single thing to have is overwhelming, and for many types of contribution you don't need anywhere near all of it. Several of the sections would make as much sense alone (e.g., Memory Management in WebKit). It's perhaps also worth questioning whether the content that's Xcode-specific belongs in the general introduction. I'd rather we landed this in parts in docs/, preferably with a plan to migrate the rest of the documentation (but not the product planning, etc.) from webkit.org and trac.webkit.org into that directory. If we don't care about preserving the wiki history, we can pretty easily get a dump of all the content in there, though I do suspect the history has some limited value when evaluating whether a page is worth keeping. Also: we probably don't need poorly compressed (zopflipng -m reduces them from 4.0MB to 2.3MB) and HiDPI screenshots for our docs.
(In reply to Sam Sneddon [:gsnedders] from comment #26) > > I'm slightly skeptical about adding a new introduction.md without formally > getting rid of the wiki (and, as you say, the wiki is a mess). We already > have documentation split across at least three places (Readme.md, > trac.webkit.org/wiki, and webkit.org) and it's not really clear how to get > the necessary permissions to edit each of these if the documentation is a > mess. You don't need to get any permission from anyone. The way to update the documentation is to post a patch to update them. FWIW, webkit.org content is under Websites/webkit.org. My plan is to eventually get rid of the entirely of wiki and some of pages in webkit.org but this documentation doesn't contain nearly enough content to to do that. In any case, we need this patch to be first landed so that enough people can take a look to see if which old documentation can be removed / replaced. > We need to have some real discussion about how to document WebKit going > forward, which I've spoken a bit with a few people on about (mostly recently > Brady and Maciej), and I'm unconvinced that adding a single, huge page to > the root of the repo is a good approach. I've successfully mentioned 4 full time engineers using this documentation earlier this year, and they all told me this was the single most useful source of information they had. Marcus and Tetsuharu also found this documentation useful. I have no doubt that there might be a better approach. However, we need to start from somewhere. > The biggest problem with the wiki > is lack of coherency and lack of any real cross-references. A single page > doesn't solve the former (especially as it gets longer), and the lack of > cross-references is only mitigated by Cmd/Ctrl+F. It does mitigate the issue of coherency issue. When everything is in a single document, it's easier to be consistent with the rest of it. > That said, I think this is too long; I don't think you need everything in > here to write a WebKit patch. An introduction to every single thing to have > is overwhelming, and for many types of contribution you don't need anywhere > near all of it. Several of the sections would make as much sense alone > (e.g., Memory Management in WebKit). It's perhaps also worth questioning > whether the content that's Xcode-specific belongs in the general > introduction. Again, the fact 4 full time engineers who worked on the WebKit project earlier this year as well as Marcus and Tetsuharu found this introduction document useful are direct empirical evidence that supports in the favor of adding this documentation. What empirical evidence do you have that it doesn't? Also, in my point of view, this documentation doesn't nearly have enough content. We need more information about how bindings work, how DOM nodes' various functions need to behave, etc... > I'd rather we landed this in parts in docs/, preferably with a plan to > migrate the rest of the documentation (but not the product planning, etc.) > from webkit.org and trac.webkit.org into that directory. I don't want to do that because splitting this into multiple pieces is a sure way for people to get lost in figuring out what they need to know. The hardest part of contributing to WebKit is to know what you don't know. I've seen numerous examples of that over the last ~12 years of helping people getting started in the WebKit project. People don't realize that they need to learn about something so they forget to do something or make wrong assumptions. Splitting this document into multiple pieces would introduce that problem. > If we don't care > about preserving the wiki history, we can pretty easily get a dump of all > the content in there, though I do suspect the history has some limited value > when evaluating whether a page is worth keeping. No, we don't want to do that. Most of contents in the wiki are extremely outdated and incorrect. And I have no desire to review & update all those old documents because they're not written or organized in a coherent manner. > Also: we probably don't need poorly compressed (zopflipng -m reduces them > from 4.0MB to 2.3MB) and HiDPI screenshots for our docs. I'm not certain that I'm allowed to use that particular image compressor but if someone wants to do that for me, and upload the compressed images, I can incorporate into the patch. Alternatively, I can land this and someone can do that after the fact. I see Don @ Sony does this across multiple directories every now and then so maybe he can help us here.
<rdar://problem/69922014>
Committed r269209: <https://trac.webkit.org/changeset/269209>