Skip to content

Dfl 3356 maintenance #102

Open
wants to merge 32 commits into from

4 participants

@chriskr
Opera Software member
chriskr commented Oct 2, 2012

No description provided.

chriskr added some commits Jul 19, 2012
@chriskr chriskr DFL-3226 Lots of info messages "getting readyState has failed in runt…
…ime_onload_handler handleReadyState"
7ce75f8
@chriskr chriskr DFL-1360 Remeber selected element in DOM view between reloads. 345b282
@chriskr chriskr DFL-3358 The tooltip in Scripts must take the runtime of the script t…
…o evaluate a selection.
95ab36e
@chriskr chriskr Code cleanup. 1a05029
@chriskr chriskr Code cleanup and select the runtime on selecting a script. 27245e8
@chriskr chriskr Added new method to find specific tokens. 882f570
@chriskr chriskr DFL-3360 Stylesheet is missing when debugging certain testcase. dd59a2a
@chriskr chriskr DFL-3216 Getting the execution context must be smarter. e64a13d
@chriskr chriskr Run cleanrepo. d2bec3e
@chriskr chriskr Synced with master. 263f01b
@chriskr chriskr Run cleanrepo. 0769f67
@chriskr chriskr Merge branch 'master' into DFL-3356_maintenance a69c05a
@chriskr chriskr Working on DFL-3159. c0ece04
@chriskr chriskr Adjusting tooltips according to DFL-3159. 96fe64d
@chriskr chriskr Set the correct flag. b41252d
@chriskr chriskr Working on copy action. 0723518
@chriskr chriskr Added more copy actions. 1892324
@chriskr chriskr Added 'Copy CSS path' action. 361dc12
@chriskr chriskr Cleanup serialze to string of a DOM node. 4fbf526
@chriskr chriskr Fixd some issues with markup edit. 4e76486
@chriskr chriskr Added SELECTION menu type. 689eb92
@chriskr chriskr Copy script source. 2958762
@chriskr chriskr Updated message definitions and made them collapsible. 3205245
@chriskr chriskr Added missing default method. f7db753
@chriskr chriskr Removed the clipboard API hack. f9fe9b9
@chriskr chriskr Log action handlers with missing get_selection_string method. 6bdb240
@chriskr chriskr Removed empty line. 0ef4161
@chriskr chriskr Fixed syntax error. efb22c8
@chriskr chriskr Fixed merge conflicts with master. 15f63e3
@chriskr chriskr Fixed merge conflicts with cutting-edge. 99ab7b1
@chriskr
Opera Software member
chriskr commented Oct 2, 2012

Don't review this now, will make a separate pull request only for the update of the message definitions.

@chriskr
Opera Software member
chriskr commented Oct 3, 2012

Still not a small review, lot of cleanup, but now it is ready.

@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/actions.js
@@ -497,9 +499,10 @@ cls.DOMInspectorActions = function(id)
this.blur = function(event)
{
+
@danfooo
danfooo added a note Oct 4, 2012

Extra newline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/actions.js
if (this.mode != MODE_DEFAULT && this.editor)
this.editor.submit();
- if (selection)
+ if (selection && !selection.isCollapsed)
@danfooo
danfooo added a note Oct 4, 2012

Newline before

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/actions.js
@@ -892,6 +889,48 @@ cls.DOMInspectorActions = function(id)
}
}.bind(this);
+ this._handlers["copy-xpath"] = function(event, target)
+ {
+ var model = window.dominspections[target.get_ancestor_attr("data-model-id")];
+ var obj_id = parseInt(target.get_ancestor_attr("ref-id"));
@danfooo
danfooo added a note Oct 4, 2012

Think we mostly use Number when getting values form attributes, in the following functions too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/lib/clipboard.js
+var Clipboard = function() {};
+
+(function()
+{
+ var _content_editable_ele = null;
+ var _is_supported = false;
+ var _is_copy_action = false;
+
+ /* static methods */
+
+ this.set_string = function(string)
+ {
+ _is_copy_action = true;
+ var listener = _set_string.bind(null, string);
+ document.addEventListener("copy", listener, false);
+ document.execCommand("copy");
@danfooo
danfooo added a note Oct 4, 2012

I have seen this throwing SECURITY_ERR, I guess that depends on the "Let Site Access Clipboard" setting. Will that stay this way? We should probably have feature detection that sets a flag to offer clipboard dependent features. This might also be different when Dragonfly is used from file://?

Ah, there is _test_support, but I got a false positive it seems. Will check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/domdata.js
@@ -457,6 +468,22 @@ cls.EcmascriptDebugger["6.0"].DOMData = function(view_id)
'this._handle_snapshot in dom_data has failed');
}
+ this._on_element_selected = function(msg)
+ {
+ var win_id = window.runtimes.getActiveWindowId();
+ var css_path = this.get_css_path(msg.obj_id, null, true, false, true);
+ var is_first = false;
+ var selector = css_path.reduce(function(sel, ele)
+ {
@danfooo
danfooo added a note Oct 4, 2012

Inline function should have { on the previous line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/domdata.js
@@ -457,6 +468,22 @@ cls.EcmascriptDebugger["6.0"].DOMData = function(view_id)
'this._handle_snapshot in dom_data has failed');
}
+ this._on_element_selected = function(msg)
+ {
+ var win_id = window.runtimes.getActiveWindowId();
+ var css_path = this.get_css_path(msg.obj_id, null, true, false, true);
+ var is_first = false;
+ var selector = css_path.reduce(function(sel, ele)
+ {
+ sel = sel + ele.name + (is_first ? ":first-child " : " ") +
@danfooo
danfooo added a note Oct 4, 2012

sel +=?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/inspectabledomnode.js
+ this.get_unique_css_path = function(object_id, force_lower_case)
+ {
+ return this._get_unique_path("CSS", object_id, force_lower_case, true);
+ };
+
+ this._get_unique_path = function(type, object_id, force_lower_case, is_in_xhtml_ns)
+ {
+ var path = "";
+ var name = "";
+ var name_count = 0;
+ var ele = null;
+ var prev_ele = null;
+ var selector = this._selectors[type];
+ if (object_id)
+ {
+ for (var i = 0; (ele = this._data[i]) && ele[ID] != object_id; i++);
@danfooo
danfooo added a note Oct 4, 2012

At first I though it would be nice to filter here instead, but i is used further down, so that won't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/inspectabledomnode.js
+ var id = is_in_xhtml_ns && this.get_attr(ele, "id");
+ if (id)
+ return selector.get_id(id);
+ name = this._get_element_name(ele, force_lower_case);
+ name_count = 1;
+ }
+ prev_ele = ele;
+ i--;
+ for ( ; ele = this._data[i]; i--)
+ {
+ if (ele[TYPE] == Node.ELEMENT_NODE)
+ {
+ if (ele[DEPTH] < prev_ele[DEPTH])
+ {
+ path = selector.get_part(name, name_count) + path;
+ name = this._get_element_name(ele, force_lower_case);
@danfooo
danfooo added a note Oct 4, 2012

name =

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/markupeditor.js
@@ -64,18 +51,17 @@ var DOMMarkupEditor = function()
model: model
};
this.context_cur = {};
- for( prop in this.context_enter )
+ for (var prop in this.context_enter )
@danfooo
danfooo added a note Oct 4, 2012

Extra space before )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/markupeditor.js
@@ -163,6 +149,8 @@ var DOMMarkupEditor = function()
{
this.textarea.style.height = 0;
this.textarea.style.height = this.textarea.scrollHeight + 'px';
+ this.textarea_container.offsetHeight;
@danfooo
danfooo added a note Oct 4, 2012

Probably worth a comment what this does.

@p01
Opera Software member
p01 added a note Oct 31, 2012

This particular line triggers a reflow, but I don't quite get the purpose of this.textarea.style.height = this.textarea.scrollHeight + 'px';afterwards. Is it to cope with some painting issue ?

@chriskr
Opera Software member
chriskr added a note Oct 31, 2012

Not painting issue. Without the following line it could end up with a scrollbar for the textarea. Couldn't find out a less expensive sequence with the same result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/inspectabledomnode.js
+ {
+ if (status != SUCCESS || message[STATUS] != "completed")
+ return callback(null);
+ var is_in_xhtml_ns = message[VALUE] == "true";
+ var path = this._get_unique_path("XPath", object_id, force_lower_case, is_in_xhtml_ns);
+ callback(path);
+ };
+
+ this.get_unique_css_path = function(object_id, force_lower_case)
+ {
+ return this._get_unique_path("CSS", object_id, force_lower_case, true);
+ };
+
+ this._get_unique_path = function(type, object_id, force_lower_case, is_in_xhtml_ns)
+ {
+ var path = "";
@hzr
hzr added a note Oct 4, 2012

Move these variables to the block where they are used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/inspectabledomnode.js
+ var name = "";
+ var name_count = 0;
+ var ele = null;
+ var prev_ele = null;
+ var selector = this._selectors[type];
+ if (object_id)
+ {
+ for (var i = 0; (ele = this._data[i]) && ele[ID] != object_id; i++);
+ if (ele)
+ {
+ if (ele[TYPE] == Node.ELEMENT_NODE)
+ {
+ var id = is_in_xhtml_ns && this.get_attr(ele, "id");
+ if (id)
+ return selector.get_id(id);
+ name = this._get_element_name(ele, force_lower_case);
@hzr
hzr added a note Oct 4, 2012

Extra space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/inspectabledomnode.js
+ for ( ; ele = this._data[i]; i--)
+ {
+ if (ele[TYPE] == Node.ELEMENT_NODE)
+ {
+ if (ele[DEPTH] < prev_ele[DEPTH])
+ {
+ path = selector.get_part(name, name_count) + path;
+ name = this._get_element_name(ele, force_lower_case);
+ name_count = 1;
+ prev_ele = ele;
+ }
+ else if (ele[DEPTH] == prev_ele[DEPTH] && ele[NAME] == prev_ele[NAME])
+ name_count++;
+ }
+ }
+ if (name)
@hzr
hzr added a note Oct 4, 2012

Newline before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/inspectabledomnode.js
+ return path.replace(/^ > /, "");
+ };
+
+ this._selectors = {};
+ this._selectors["XPath"] =
+ {
+ get_id: function(id)
+ {
+ return "//*[@id=\"" + id + "\"]";
+ },
+ get_part: function(name, name_count)
+ {
+ return "/" + name + (name_count > 1 ? "[" + name_count + "]" : "");
+ }
+ };
+ this._selectors["CSS"] =
@hzr
hzr added a note Oct 4, 2012

Newline before I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/inspectabledomnode.js
@@ -507,6 +618,22 @@ cls.EcmascriptDebugger["6.0"].InspectableDOMNode.prototype = new function()
return this._data_runtime_id;
}
+ this.serialize_to_string = function(node_id, callback)
+ {
+ var script = "ele.namespaceURI == \"http://www.w3.org/1999/xhtml\"" +
+ "? ele.outerHTML" +
@hzr
hzr added a note Oct 4, 2012

Line up with the string above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/markupeditor.js
- obj_id = parseInt(ele.parentElement.getAttribute('ref-id')),
- model_id = ele.get_attr('parent-node-chain', 'data-model-id'),
- script = '',
- tag = '',
- prop = '',
- container = ele,
- model = null,
- dom = null,
- cb = null;
-
- while( container
- && !/container/i.test(container.nodeName)
- && ( container = container.parentElement ) );
- if(container && model_id)
+ var ele = ref_ele || event.target;
+ var rt_id = parseInt(ele.get_ancestor_attr("rt-id"));
@hzr
hzr added a note Oct 4, 2012

Number(), also on the next line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/markupeditor.js
{
this.context_cur[prop] = this.context_enter[prop];
}
- script = this["return new Host_updater(target)"];
- tag = tagManager.set_callback(this, this.register_host_updater, [rt_id]);
- services['ecmascript-debugger'].requestEval(tag, [rt_id, 0, 0, script, [['target', obj_id]]]);
- dom = new cls.InspectableDOMNode(rt_id, obj_id);
- cb = this.handle_get_outer_html.bind(this, dom, rt_id, obj_id, ele, event);
- dom.expand(cb, obj_id, "subtree");
+ var script = this["return new Host_updater(target)"];
+ var tag = tagManager.set_callback(this, this.register_host_updater, [rt_id]);
@hzr
hzr added a note Oct 4, 2012

window.tag_manager.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/markupeditor.js
{
- var
- outerHTML = this.domnodeserializer.serialize(dom),
- parent = ele.parentNode,
- parent_parent = parent.parentElement,
- margin = parseInt(parent.style.marginLeft),
- next = null;
- this.context_enter.outerHTML = this.context_cur.outerHTML = outerHTML;
- if( !this.base_style['font-size'] )
- {
+ var parent = ele.parentNode;
+ var parent_parent = parent.parentElement;
@hzr
hzr added a note Oct 4, 2012

Check for parent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/dominspection/markupeditor.js
var scroll_position = new Element.ScrollPosition(parent);
parent.innerHTML = "";
parent.appendChild(this.textarea_container);
- while( ( next = parent.nextElementSibling ) && parseInt(next.style.marginLeft) > margin )
+ while ((next = parent.nextElementSibling ) && parseInt(next.style.marginLeft) > margin)
@hzr
hzr added a note Oct 4, 2012

Extra space after nextElementSibling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/runtime_onload_handler.js
{
- for (var i = 0, cb; cb = onload_handlers[i]; i++)
- {
- cb();
- }
+ callbacks.shift()()
@hzr
hzr added a note Oct 4, 2012

Missing semicolon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/runtime_onload_handler.js
// if the callback already exists, it will be replaced
- var i = 0;
- for (var cb; (cb = onload_handlers[i]) && cb != callback; i++) {};
- onload_handlers[i] = callback;
- }
+ for (var i = 0, cb; (cb = callbacks[i]) && cb != callback; i++);
+ callbacks[i] = callback;
+ if (error_callback)
+ {
+ callbacks = this._error_handlers[rt_id];
+ for (var i = 0, cb; (cb = callbacks[i]) && cb != callback; i++);
+ callbacks[i] = error_callback;
+ }
+ if (timeout)
@hzr
hzr added a note Oct 4, 2012

Newline before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/runtime_onload_handler.js
- messages.addListener("thread-continue-event", onThreadContinue);
- messages.addListener('reset-state', reset_state_handler);
- window.services['ecmascript-debugger'].addListener('readystatechanged', this._onloadhandler.bind(this));
+ this._init = function()
+ {
+ this._rts = {};
+ this._onload_handlers = {};
+ this._error_handlers = {};
+ this._rts_checked = {};
+ this._rts_is_checking = {};
+ this._blocked_rts = {};
+ this._timeouts = {};
+ this._esde = window.services['ecmascript-debugger'];
+ this._tagman = window.tag_manager;
+ var msgs = window.messages;
+ msgs.addListener("thread-stopped-event", this._on_thread_stopped.bind(this));
@hzr
hzr added a note Oct 4, 2012

add_listener.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
}
- if( debug_context_frame_path == runtime.html_frame_path &&
- __selected_window == runtime.window_id &&
- runtimeId != __selected_runtime_id )
+
+ if (_windows_reloaded[runtime.window_id] == 1)
@hzr
hzr added a note Oct 4, 2012

Should the 1 and 2 be constants?

@danfooo
danfooo added a note Oct 4, 2012

Explaining this a bit would help, I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtime_onload_handler.js
// if the callback already exists, it will be replaced
- var i = 0;
- for (var cb; (cb = onload_handlers[i]) && cb != callback; i++) {};
- onload_handlers[i] = callback;
- }
+ for (var i = 0, cb; (cb = callbacks[i]) && cb != callback; i++);
+ callbacks[i] = callback;
+ if (error_callback)
+ {
+ callbacks = this._error_handlers[rt_id];
+ for (var i = 0, cb; (cb = callbacks[i]) && cb != callback; i++);
+ callbacks[i] = error_callback;
+ }
+ if (timeout)
+ {
+ var handler = this._timeout_handler.bind(this, rt_id, callback);
@danfooo
danfooo added a note Oct 4, 2012

Should this maybe use error_callback? When it times out I would expect that to be called, not the default one. Edit: Ah, or maybe timeout here is more like a delay?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
- __windows_reloaded = {};
- __selected_script = '';
- /*
- if( _is_first_call_create_all_runtimes_on_debug_context_change )
- {
- stop_at.setInitialSettings();
- // with the STP 1 design this workaround can be removed
- _is_first_call_create_all_runtimes_on_debug_context_change = false;
- }
- */
- var tag = tagManager.set_callback(null, set_new_debug_context, [win_id]);
- ecma_debugger.requestListRuntimes(tag, [[],1]);
+ _debug_context_frame_path = '';
+ _windows_reloaded = {};
+ this.setSelectedScript();
+ var tag = tagManager.set_callback(this, this._set_new_debug_context, [win_id]);
@hzr
hzr added a note Oct 4, 2012

Extra space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtime_onload_handler.js
- window.services['ecmascript-debugger'].addListener('readystatechanged', this._onloadhandler.bind(this));
+ this._init = function()
+ {
+ this._rts = {};
+ this._onload_handlers = {};
+ this._error_handlers = {};
+ this._rts_checked = {};
+ this._rts_is_checking = {};
+ this._blocked_rts = {};
+ this._timeouts = {};
+ this._esde = window.services['ecmascript-debugger'];
+ this._tagman = window.tag_manager;
+ var msgs = window.messages;
+ msgs.addListener("thread-stopped-event", this._on_thread_stopped.bind(this));
+ msgs.addListener("thread-continue-event", this._on_thread_continue.bind(this));
+ msgs.addListener('reset-state', this._reset_state_handler.bind(this));
@danfooo
danfooo added a note Oct 4, 2012

Sudden change to single quotes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
// sub message RuntimeInfo
- RUNTIME_ID = 0,
- HTML_FRAME_PATH = 1,
- WINDOW_ID = 2,
- OBJECT_ID = 3,
- URI = 4,
- DESCRIPTION = 5,
- THREAD_STARTED = 0,
- THREAD_STOPPED_AT = 1,
- THREAD_FINISHED = 2;
-
+ var RUNTIME_ID = 0;
@danfooo
danfooo added a note Oct 4, 2012

If these are for messages, why not use the enums from the generated classes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
- var __selected_runtime_id = '';
-
- var __next_runtime_id_to_select = '';
-
- var __selected_script = '';
- var __selected_script_type = '';
-
+ var _runtimes = {};
+ var _rt_class = cls.EcmascriptDebugger["6.0"].Runtime;
+ var _dom_rt_class = cls.EcmascriptDebugger["6.0"].DOMRuntime;
+ var _ext_rt_class = cls.EcmascriptDebugger["6.0"].ExtensionRuntime;
+ var _old_runtimes = {};
+ var _runtime_ids = [];
+ var _window_ids = {};
+ var _windows_reloaded = {};
+ var _selected_window = '';
@danfooo
danfooo added a note Oct 4, 2012

Single quotes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
{
if (msg.profile == window.app.profiles.DEFAULT)
{
- __runtimes = {};
- __old_runtimes = {};
- __runtimes_arr = []; // runtime ids
- __window_ids = {};
- __windows_reloaded = {};
- __threads = [];
- __log_threads = false;
- __windowsFolding = {};
- __selected_runtime_id = '';
- __next_runtime_id_to_select = '';
- __selected_script = '';
- current_threads = {};
- updateRuntimeViews();
+ _runtimes = {};
@danfooo
danfooo added a note Oct 4, 2012

Isn't that what _on_reset_state is for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
this.runtime_has_dom = function(rt_id)
{
// description is only available in newer Core versions, so if it's undefined it has DOM
- return __runtimes[rt_id] && (__runtimes[rt_id].description == "document" ||
- __runtimes[rt_id].description === undefined);
+ return _runtimes[rt_id] && (_runtimes[rt_id].description == "document" ||
+ _runtimes[rt_id].description === undefined);
@danfooo
danfooo added a note Oct 4, 2012

Align with _runtimes above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
@@ -760,20 +625,19 @@ cls.EcmascriptDebugger["6.0"].Runtimes = function(service_version)
// TODO client side therads handling needs a revision
@danfooo
danfooo added a note Oct 4, 2012

Does it still, after these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
this.onThreadStarted = function(status, message)
{
-
- const
- RUNTIME_ID = 0,
- THREAD_ID = 1,
- PARENT_THREAD_ID = 2,
- THREAD_TYPE = 3,
- EVENT_NAMESPACE = 4,
- EVENT_TYPE = 5;
-
+ var RUNTIME_ID = 0;
@danfooo
danfooo added a note Oct 4, 2012

Maybe use the generated ThreadStarted class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
opera.postError(ui_strings.S_DRAGONFLY_INFO_MESSAGE +
- 'thread finished not in debug context')
@danfooo
danfooo added a note Oct 4, 2012

Good to see this go! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
{
- return __scripts[scriptId].script_data
+ return _scripts[scriptId].script_data
@danfooo
danfooo added a note Oct 4, 2012

Missing semicolon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ecma-debugger/runtimes.js
+ messages.addListener('profile-disabled', this._on_profile_disabled.bind(this));
+ messages.addListener('profile-enabled', this._on_profile_enabled.bind(this));
+
+ this.bind = function(_ecma_debugger)
+ {
+ _ecma_debugger.handleEval = function(status, message) {};
+ _ecma_debugger.handleListRuntimes = this.handleListRuntimes.bind(this);
+ _ecma_debugger.onRuntimeStarted = this.onRuntimeStarted.bind(this);
+ _ecma_debugger.onRuntimeStopped = this.onRuntimeStopped.bind(this);
+ _ecma_debugger.onNewScript = this.onNewScript.bind(this);
+ _ecma_debugger.onThreadStarted = this.onThreadStarted.bind(this);
+ _ecma_debugger.onThreadStoppedAt = this.onThreadStoppedAt.bind(this);
+ _ecma_debugger.onThreadFinished = this.onThreadFinished.bind(this);
+ _ecma_debugger.onThreadMigrated = this.onThreadMigrated.bind(this);
+ _ecma_debugger.onParseError = this.onParseError.bind(this);
+ // TODO looks strange
@danfooo
danfooo added a note Oct 4, 2012

Yes, looks like a shortcut

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/profiler/profiler_view.js
@@ -413,8 +413,8 @@ var ProfilerView = function(id, name, container_class, html, default_handler)
this._old_session_id = null;
this._reset();
- Tooltips.register("profiler-tooltip-url", true, false);
- this._tooltip = Tooltips.register("profiler-event", true, false);
+ Tooltips.register("profiler-tooltip-url", {type: Tooltips.TYPE_SUPPORT_CONTEXT});
@danfooo
danfooo added a note Oct 4, 2012

We always say Tooltips without defining that as window.Tooltips first, right? Not really consequent.

@hzr
hzr added a note Oct 4, 2012

This is fine and preferred for classes according to the style guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/repl/repl_service.js
var magicvars = [];
- if (this._cur_selected)
- magicvars.push(["$0", this._cur_selected]);
-
- if (this._prev_selected)
- magicvars.push(["$1", this._prev_selected]);
+ if ($_identifiers.contains("$0") && this._cur_selected)
+ {
+ magicvars.push(["$0", this._cur_selected.obj_id]);
+ if (this._cur_selected.rt_id != rt_id)
@danfooo
danfooo added a note Oct 4, 2012

rt_id is always undefined here it seems. Should probably be defined before, and the checks should be skipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/repl/repl_service.js
+ if ($_identifiers.contains("$1") && this._prev_selected)
+ {
+ magicvars.push(["$1", this._prev_selected.obj_id]);
+ if (this._prev_selected.rt_id != rt_id)
+ this._runtime_select.set_id(this._prev_selected.rt_id)
@danfooo
danfooo added a note Oct 4, 2012

If we had $0 and $1, we will execute in $1's runtime? I think $0 should take precedence, that's the most recently selected element, I wouldn't expect overriding because the previously selected was on some other runtime.

I realise both need to be actually used for this to show up, but I still think $0 should win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/style/view_color_picker.js
this._tooltip.ontooltip = function(event, target) {
var box = target.getBoundingClientRect();
- box.mouse_x = box.left;
+ box.mouse_x = box.left - 5;
@danfooo
danfooo added a note Oct 4, 2012

What is this? Should probably a CONST, or maybe be read from styles?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-scripts/contextmenu.js
- });
- this.register("spec", items);
+ items = specs.map(function(spec)
+ {
+ return {
+ label: ui_strings.M_CONTEXTMENU_SPEC_LINK.replace("%s", spec.prop),
+ handler: function(event, target) {
+ speclinks.open_spec_link(spec.url);
+ },
+ id: spec.prop,
+ menu_id: "spec"
+ };
+ });
+ this.register("spec", items);
+
+ if (all_items.length)
@danfooo
danfooo added a note Oct 4, 2012

Extra newline & can skip braces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-scripts/contextmenu.js
{
- all_items.push(ContextMenu.separator);
+ all_items = all_items.concat(items);
@danfooo
danfooo added a note Oct 4, 2012

all_items.extend(items);?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-scripts/contextmenu.js
}
- }
- else
- {
- var fun = function()
+ var broker = cls.ResourceDisplayBroker.get_instance();
+ var rid = parseInt(res_id_or_url, 10);
@danfooo
danfooo added a note Oct 4, 2012

Probably just Number here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-scripts/contextmenu.js
}
- )
+
+ all_items.push(
+ {
+ label: ui_strings.M_CONTEXTMENU_SHOW_RESOURCE,
+ handler: fun,
+ id: res_id_or_url,
+ menu_id: "resource"
+ }
+ )
@danfooo
danfooo added a note Oct 4, 2012

Missing semicolon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-scripts/tooltip/tooltip.js
{
- this._init(keep_on_hover, set_selected, max_height_target);
+ /*
+ config
+ keep_on_hover: Boolean, default false
+ set_selected: Boolean, default false
+ If true it sets the class 'selected' on the target.
+ max_height_target: String css_query
+ If set it sets max-width and max-height on that element instead
@danfooo
danfooo added a note Oct 4, 2012

based on that element?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-scripts/tooltip/tooltip.js
{
- this._init(keep_on_hover, set_selected, max_height_target);
+ /*
+ config
+ keep_on_hover: Boolean, default false
+ set_selected: Boolean, default false
+ If true it sets the class 'selected' on the target.
+ max_height_target: String css_query
+ If set it sets max-width and max-height on that element instead
+ on the tooltip-container.
+ dynamic_size: Boolean, default false
+ If true it positions the tooltip always in the biggest possible free area.
+ If false the preferred position is bottom-right of the target. Only if it
+ doesn't fit there it positions it in an other area.
+ class: String default ""
@danfooo
danfooo added a note Oct 4, 2012

Think this should be called classname, a bit more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-scripts/tooltip/tooltip.js
@@ -278,6 +325,12 @@ Tooltips.CSS_TOOLTIP_SELECTED = "tooltip-selected";
_cur_ctx.tooltip_ele.innerHTML = "";
+ if (_cur_ctx.current_tooltip.class)
+ _cur_ctx.tooltip_ele.addClass(_cur_ctx.current_tooltip.class);
+ else
+ _cur_ctx.tooltip_ele.className = "tooltip-container";
@danfooo
danfooo added a note Oct 4, 2012

Doesn't it have tooltip-container by default, out of the template? A bit strange that one case adds a classname, the other one resets it. Is the container reused?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-strings/ui_strings-en.js
@@ -915,9 +933,6 @@ ui_strings.S_INFO_DOCUMNENT_LOADING = "Updating Opera Dragonfly…";
/* DESC: There was an error trying to listen to the specified port */
ui_strings.S_INFO_ERROR_LISTENING = "There was an error. Please check that port %s is not in use.";
-/* DESC: A info message that the debugger is currently in HTTP profiler mode. */
-ui_strings.S_INFO_HTTP_PROFILER_MODE = "The debugger is in HTTP profiler mode. All other features are disabled.";
@danfooo
danfooo added a note Oct 4, 2012

How come this is removed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danfooo danfooo commented on the diff Oct 4, 2012
src/ui-style/ui.css
-{
- display: block;
- line-height: 1.1;
- text-align: center;
- overflow: hidden;
- text-overflow: ellipsis;
-}
-
-.background-overlay
-{
- position: absolute;
- border: 1px solid #999;
- box-shadow: 0 5px 5px hsla(0, 0%, 0%, 0.30),
- 0 2px 2px hsla(0, 0%, 0%, 0.25)
-}
+/* Font stack defaults */
@danfooo
danfooo added a note Oct 4, 2012

Hard to see what the actual changes are here. Has 5 line more than before though :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 25, 2012
src/repl/repl_service.js
@@ -20,6 +20,12 @@ cls.ReplService = function(view, data)
const RE_DOM_OBJECT = cls.InlineExpander.RE_DOM_OBJECT;
const IS_EXPAND_INLINE_KEY = "expand-objects-inline";
const CLASS_NAME = 4;
+ var ELEMENT_IDENTIFIERS =
@hzr
hzr added a note Oct 25, 2012

Bracket on same line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 25, 2012
src/ui-scripts/actions/actionbroker.js
window.messages.post('shortcuts-changed');
}.bind(this));
};
+ this._oncopy = function(event)
+ {
+ var selection = this._action_context && this._action_context.get_selection_string();
+ if (selection)
+ {
+ event.preventDefault();
+ Clipboard.set_string(selection);
+ }
+
@hzr
hzr added a note Oct 25, 2012

Extra newline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 25, 2012
src/ui-scripts/contextmenu.js
@@ -108,6 +112,19 @@ function ContextMenu() {
var last_found_menu_id = '';
var collected_menus = [];
var items = null;
+ var selection = window.getSelection();
+ var range = Clipboard.is_supported && !selection.isCollapsed && selection.getRangeAt(0);
+ var cur_handler_id = ActionBroker.get_instance().get_current_handler_id();
+ var view = window.views[cur_handler_id];
+ if (view && !view.get_selection_string)
+ {
+ opera.postError(ui_strings.S_DRAGONFLY_INFO_MESSAGE +
+ " action handler without get_selection_string method: " + view_id + ".");
+
@hzr
hzr added a note Oct 25, 2012

Extra newline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 25, 2012
src/ui-scripts/view.js
@@ -255,6 +255,11 @@ var ViewBase = new function()
}
+ this.get_selection_string = function()
@hzr
hzr added a note Oct 25, 2012

"Virtual" function? Would put everything on the same line like usual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr hzr commented on the diff Oct 25, 2012
src/ui-strings/ui_strings-en.js
@@ -45,6 +45,24 @@ ui_strings.M_CONTEXTMENU_ADD_WATCH = "Watch \"%s\"";
/* DESC: Context menu item for collapsing a node subtree. */
ui_strings.M_CONTEXTMENU_COLLAPSE_SUBTREE = "Collapse subtree";
+/* DESC: Generic context menu item to copy something. */
+ui_strings.M_CONTEXTMENU_COPY = "Copy";
+
+/* DESC: Context menu item to copy content, e.g of a file. */
+ui_strings.M_CONTEXTMENU_COPY_CONTENT = "Copy content";
+
+/* DESC: Context menu item to copy the CSS path. */
+ui_strings.M_CONTEXTMENU_COPY_CSS_PATH = "Copy CSS path";
@hzr
hzr added a note Oct 25, 2012

s/CSS path/selector, also in the comment.

@danfooo
danfooo added a note Oct 25, 2012

I agree. Too late for translations, but at least for the english one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hzr
hzr commented Oct 25, 2012

Finished review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.