[Conkeror] [PATCH] Make window.js aware of which windows are uninitialized. Use this to make
Jeremy Maitin-Shepard
jeremy at jeremyms.com
Fri Sep 5 18:59:07 PDT 2008
David House <dmhouse at gmail.com> writes:
See below for my comments. Other than the issues I pointed out, this
seems good to go in.
> ---
> modules/buffer.js | 65 +++++++++++++++-------------------------------------
> modules/window.js | 64 +++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 62 insertions(+), 67 deletions(-)
> diff --git a/modules/buffer.js b/modules/buffer.js
> index f405730..c0085fc 100644
> --- a/modules/buffer.js
> +++ b/modules/buffer.js
> @@ -51,18 +51,6 @@ function buffer_creator(type) {
> }
> }
> -/* NOTE this code is horifically duplicated from
> -create_buffer_in_current_window. The correct way to fix this is to make
> -window.js be aware of the currently uninitialised windows, which isn't too hard
> -but I just haven't done it yet. */
> -var mbc_queued_buffer_creators = {};
> -function mbc_process_queued_buffer_creators(window) {
> - for (var i = 0; i < mbc_queued_buffer_creators[window.tag].length; i++) {
> - mbc_queued_buffer_creators[window.tag][i](window, null);
> - }
> - mbc_queued_buffer_creators[window.tag] = null;
> -}
> -
It would help if you made this patch against the Conkeror upstream
master, rather than against your previous patch.
> /**
> * A buffer creator to make many buffers at once. Example:
> *
> @@ -77,17 +65,11 @@ function multiple_buffers_creator(creators) {
> // nothing else is specified
> if (creators.length == 0)
> return new content_buffer(window, element, $load = homepage);
> -
> - for (var i = 0; i < creators.length; i++) {
> - if (typeof mbc_queued_buffer_creators[window.tag] != "undefined") {
> - mbc_queued_buffer_creators[window.tag].push(creators[i]);
> - } else {
> - mbc_queued_buffer_creators[window.tag] = [];
> - window.buffers.current = creators[i](window, element);
> - add_hook.call(window, "window_initialize_late_hook",
> - mbc_process_queued_buffer_creators);
> - }
> - }
> +
> + creators.shift()(window, element); // Create the first directly
> + creators.forEach(function (cr) {
> + create_buffer(window, cr, OPEN_NEW_BUFFER_BACKGROUND);
> + });
> }
> }
It seems like this stuff doesn't really belong in this patch.
> @@ -553,13 +535,20 @@ interactive_context.prototype.browse_target = function
> (action) {
> return targets[index];
> };
> +/**
> + * Open a buffer on `window'. If that window isn't fully initialized, queue the
> + * buffer for creation on `window_initialize_late_hook'.
> + */
> function create_buffer(window, creator, target) {
> switch (target) {
> case OPEN_NEW_BUFFER:
> - window.buffers.current = creator(window, null);
> - break;
> case OPEN_NEW_BUFFER_BACKGROUND:
> - creator(window, null);
> + var mk_buffer = function (w) {
> + var buf = creator(w, null);
> + if (target == OPEN_NEW_BUFFER) window.buffers.current = buf;
> + }
> + if (window.initialized) mk_buffer(window);
> + else add_hook.call(window, "window_initialize_late_hook", mk_buffer);
> break;
> case OPEN_NEW_WINDOW:
> make_window(creator);
> @@ -569,29 +558,13 @@ function create_buffer(window, creator, target) {
> }
> }
> -var queued_buffer_creators = null;
> -function _process_queued_buffer_creators(window) {
> - for (var i = 0; i < queued_buffer_creators.length; ++i) {
> - var x = queued_buffer_creators[i];
> - create_buffer(window, x[0], x[1]);
> - }
> - queued_buffer_creators = null;
> -}
> function create_buffer_in_current_window(creator, target, focus_existing) {
> if (target == OPEN_NEW_WINDOW)
> throw new Error("invalid target");
> - var window = get_recent_conkeror_window();
> - if (window) {
> - if (focus_existing)
> - window.focus();
> - create_buffer(window, creator, target);
> - } else if (queued_buffer_creators != null) {
> - queued_buffer_creators.push([creator,target]);
> - } else {
> - queued_buffer_creators = [];
> - window = make_window(creator);
> - add_hook.call(window, "window_initialize_late_hook",
> _process_queued_buffer_creators);
> - }
> + var window = get_recent_conkeror_window(true);
> + if (!window) return make_window(creator);
> + if (window && focus_existing) window.focus();
> + create_buffer(window, creator, target);
> }
> minibuffer_auto_complete_preferences["buffer"] = true;
> diff --git a/modules/window.js b/modules/window.js
> index c45eb08..2933fe2 100644
> --- a/modules/window.js
> +++ b/modules/window.js
> @@ -10,13 +10,11 @@ require("mode.js");
> var define_window_local_hook = simple_local_hook_definer();
> -
> define_hook("make_window_hook");
> var window_watcher =
> Cc["@mozilla.org/embedcomp/window-watcher;1"].getService(Ci.nsIWindowWatcher);
> -function generate_new_window_tag(tag)
> -{
> +function generate_new_window_tag(tag) {
> var existing = [];
> var exact_match = false;
> var en = window_watcher.getWindowEnumerator ();
> @@ -27,17 +25,20 @@ function generate_new_window_tag(tag)
> } else {
> re = new RegExp ("^(\\d+)$");
> }
> + var tags = [];
> while (en.hasMoreElements ()) {
> var w = en.getNext().QueryInterface (Ci.nsIDOMWindow);
> - if ('tag' in w) {
> - if (tag && w.tag == tag) {
> - exact_match = true;
> - continue;
> - }
> - var re_result = re.exec (w.tag);
> - if (re_result)
> - existing.push (re_result[1]);
> + if ("tag" in w) tags.push(w.tag);
> + }
> + tags = tags.concat([w.tag for (w in uninitialized_windows)]);
> + for (var i = 0; i < tags.length; i++) {
> + if (tag && tags[i] == tag) {
> + exact_match = true;
> + continue;
> }
> + var re_result = re.exec (tags[i]);
> + if (re_result)
> + existing.push (re_result[1]);
> }
> if (tag && ! exact_match)
> return tag;
> @@ -70,16 +71,26 @@ function make_chrome_window(chrome_url, args, tag) {
> var conkeror_chrome_URI = "chrome://conkeror/content/conkeror.xul";
> +var uninitialized_windows = [];
> +
> function make_window(initial_buffer_creator, tag) {
> - var args = { tag: tag,
> + var uniq_tag = generate_new_window_tag(tag);
> + var args = { tag: uniq_tag,
> initial_buffer_creator: initial_buffer_creator };
> - var window = make_chrome_window(conkeror_chrome_URI, null, tag);
> + var window = make_chrome_window(conkeror_chrome_URI, null, uniq_tag);
> + window.tag = uniq_tag;
The reason that the tag was set in window_initialize rather than here is
that in some cases (one example is for when target=_blank is used or
content JavaScript calls window.open and we decide to create a new
window) Mozilla creates the window for us, rather than make_window being
called. So you should also set the tag in window_initialize if it has
not already been set.
> window.args = args;
> + window.initialized = false;
> + uninitialized_windows.push(window);
> make_window_hook.run(window);
> return window;
> }
> -function get_window_by_tag(tag)
> +/**
> + * Fetch a window object by its unique tag. Only search the initialized
> windows,
> + * unless `uninitialized' is true.
> + */
> +function get_window_by_tag(tag, uninitialized)
> {
> var en = window_watcher.getWindowEnumerator ();
> while (en.hasMoreElements ()) {
> @@ -87,6 +98,10 @@ function get_window_by_tag(tag)
> if ('tag' in w && w.tag == tag)
> return w;
> }
> + if (typeof uninitialized != "undefined" && uninitialized) {
> + var matching = uninitialized_windows.filter(function (w) w.tag == tag);
> + if (matching.length > 0) return matching[0];
> + }
It doesn't matter too much, but note that using the filter function adds
a bit of overhead since you have a bunch of function calls and construct
an array, whereas if you just use:
for (let x = 0; x < uninitialized_windows.length; ++x) {
if (x.tag == tag)
return x;
}
it will be slightly more efficient.
> return null;
> }
> @@ -101,7 +116,7 @@ function for_each_window(func)
> }
> }
> -function get_recent_conkeror_window()
> +function get_recent_conkeror_window(uninitialized)
> {
> var window = window_watcher.activeWindow;
> if (window && ("conkeror" in window))
> @@ -112,6 +127,13 @@ function get_recent_conkeror_window()
> if ('conkeror' in window)
> return window;
> }
> + if (typeof uninitialized != "undefined" && uninitialized &&
> + uninitialized_windows.length > 0)
I suppose all of the typeof checking is to quiet warnings about
accessing undefined variables?
> + {
> + // None of these can be active yet, so just return the first we tried to
> + // create
> + return uninitialized_windows[0];
> + }
> return null;
> }
> @@ -165,11 +187,6 @@ function window_initialize(window)
> window_setup_args(window);
> - // Set tag
> - var tag = null;
> - if ('tag' in window.args) tag = window.args.tag;
> - window.tag = generate_new_window_tag(tag);
> -
As I described above, you'll still need something here.
> // Add a getBrowser() function to help certain extensions designed for
> Firefox work with conkeror
> window.getBrowser = window_get_this_browser;
> @@ -186,8 +203,13 @@ function window_initialize(window)
> },0);
> window.addEventListener("close", window_close_maybe, true /* capture */,
> false);
> -}
> + // Mark this window as initialized
> + uninitialized_windows =
> + uninitialized_windows.filter(function (w) w.tag != window.tag);
> + window.initialized = true;
> +}
> +
I think this should instead be done just after running the
window_initialize_late_hook.
> define_window_local_hook("window_before_close_hook", RUN_HOOK_UNTIL_FAILURE);
> define_window_local_hook("window_close_hook", RUN_HOOK);
> function window_close_maybe(event) {
--
Jeremy Maitin-Shepard
More information about the Conkeror
mailing list